fix(discord): PDF uploads, result_summary default, attachment diagnostics

[oheckmann74]

Summary

Addresses Ed's feedback on PRs 194 and 197:

• PDF uploads failing in DMs: Discord can return null for contentType on attachments (especially in DMs). The code fell back to application/octet-stream which was categorized as "unsupported" and silently dropped. Now guesses MIME type from file extension (.pdf → application/pdf) before falling back.

• result_summary default flipped: PR 194 intentionally changed result_summary from false to true, but this is a behavior change that makes all agents show cost/token summary embeds by default. Reverted to false and synced the manager's hardcoded fallback defaults to match.

• Silent attachment failures: When a user sends attachments but attachments.enabled is false, or when attachments are skipped (wrong type, too large, no working_directory), the logs were at debug level — invisible in normal operation. Upgraded to warn so operators can diagnose issues.

Note on acknowledge_emoji

Ed noted it appeared unimplemented, but it is fully wired: schema → manager → connector → Discord API. The implementation reads the emoji from config and calls message.react(). No changes needed.

Test plan

• pnpm typecheck passes
• Discord tests pass (346/346 including new _guessContentType tests)
• Core schema test updated for result_summary: false default
• Manual test: send PDF via Discord DM with attachments.enabled: true

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Per-agent dynamic self-scheduling: create/list/update/delete schedules.
  • Discord: file attachments (inline text/code; images/PDFs saved), voice transcription, expanded slash commands, richer embed responses, session and usage views, and file upload support.
  • Slack: file attachment support and multimodal prompt handling.
  • Multimodal prompt support across runtimes (text + image blocks).

• Bug Fixes

  • Discord: improved PDF MIME handling.

• Documentation

  • Updated Discord integration docs and added Beads (bd) integration guide.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds agent self-scheduling with per-agent dynamic schedules and a scheduler MCP; multimodal prompt support across runtimes and host-side MCP bridging; extensive Discord enhancements (attachments, voice transcription, new slash commands, embeds, command registration); Slack attachment propagation; many new tests, schema/type expansions, and backup metadata files.

Changes

Cohort / File(s)	Summary
Backup files .beads/backup/backup_state.json, .beads/backup/config.jsonl, .beads/backup/dependencies.jsonl, .beads/backup/events.jsonl, .beads/backup/issues.jsonl	New JSON/JSONL backup metadata and records added — data only, no runtime logic changes.
Docs & changesets .changeset/*, AGENTS.md, docs/src/content/docs/integrations/discord.mdx, packages/discord/README.md	Documentation and changeset entries for self-scheduling, CLI MCP injection, Discord attachments/PDF fixes, and beads/bd integration.
Config schema & exports packages/core/src/config/schema.ts, packages/core/src/config/index.ts, packages/core/src/config/merge.ts	Adds SelfScheduling and Discord features (attachments, voice, command_registration, skills), McpServer fields (headers, host), and merges mcp_servers in defaults.
Self-scheduling core packages/core/src/config/self-scheduling.ts, packages/core/src/scheduler/dynamic-schedules.ts, packages/core/src/mcp/scheduler-mcp.ts, packages/core/src/scheduler/index.ts	New per-agent dynamic schedule store (YAML), CRUD + TTL/min-interval validation, scheduler integration, and a standalone herdctl-scheduler MCP server exposing schedule tools and error types.
Scheduler & fleet integration packages/core/src/scheduler/scheduler.ts, packages/core/src/fleet-manager/...	Scheduler periodically reloads dynamic schedules and merges with static ones; FleetManager injects scheduler MCP on init and on config reload.
Runner types & prompt handling packages/core/src/runner/types.ts, packages/core/src/runner/index.ts, packages/core/src/runner/runtime/interface.ts	Adds multimodal types (TextBlock/ImageBlock/ContentBlock/PromptContent), extractPromptText, and systemPromptAppend support across runner interfaces.
Runtime & container/CLI changes packages/core/src/runner/runtime/cli-runtime.ts, .../sdk-runtime.ts, .../docker-sdk-wrapper.js, .../container-manager.ts, .../container-runner.ts, .../docker-config.ts	CLI: starts MCP HTTP bridges for injected MCP servers, synthetic result aggregation, promptText logging. SDK/Docker: multimodal prompt support; persistent container reuse; host-side MCP bridging partitioning; default max containers reduced.
MCP host bridge packages/core/src/runner/runtime/mcp-host-bridge.ts	New module to spawn/connect host-side stdio MCP servers, enumerate tools, and expose injected tool handlers for host→container bridging.
Job hooks & trigger options packages/core/src/fleet-manager/job-control.ts, packages/core/src/runner/job-executor.ts	Adds TriggerOptions fields (onJobCreated, systemPromptAppend) and normalizes prompts via extractPromptText before job creation.
Dynamic schedule tests packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts	Extensive tests covering path helpers, CRUD, TTL cleanup, validations, limits, and multi-agent loading.
Self-scheduling tests packages/core/src/config/__tests__/self-scheduling.test.ts	Comprehensive tests for injectSchedulerMcpServers env injection and system_prompt injection behavior.
Runner & MCP tests packages/core/src/runner/runtime/__tests__/*	New tests for CLI synthetic result aggregation and MCP host bridge behavior.
Discord types & connector packages/discord/src/types.ts, packages/discord/src/discord-connector.ts, packages/discord/src/index.ts, packages/discord/src/auto-mode-handler.ts	Connector options extended with commandActions/commandRegistration, uploadFile API, richer message event shape (attachments, voice), reaction/replyWithRef helpers, and guild default channel-mode fallback. Public exports expanded (normalization, transcribe types).
Discord manager & embeds packages/discord/src/manager.ts, packages/discord/src/embeds.ts, packages/discord/src/message-normalizer.ts	Major manager overhaul: per-channel run/usage tracking, streaming/deltas, attachment processing/cleanup, voice transcription wiring, tool/progress embedding; new embed builders and message normalization types.
Discord attachments & voice packages/discord/src/voice-transcriber.ts, packages/discord/src/__tests__/attachments.test.ts	OpenAI Whisper-based transcriber module and comprehensive attachment handling tests (MIME categorization, downloads, inline text, cleanup).
Discord commands & tests packages/discord/src/commands/*, packages/discord/src/commands/__tests__/*	Adds many slash commands (ping, config, tools, usage, skills, skill, session, new, stop, cancel, retry), autocomplete, guild-scoped registration, embed-based replies, and broad test updates.
Slack attachment support packages/slack/src/*	Slack now carries file attachments into events and builds PromptContent (image ContentBlocks) for prompts; types updated for attachment metadata.
Misc & housekeeping .gitignore, packages/core/package.json, various tests	Added Dolt ignore rules, dependency @modelcontextprotocol/sdk, and many tests updated to include self_scheduling and new config fields.

Sequence Diagrams

sequenceDiagram
    actor Agent
    participant FleetMgr as Fleet Manager
    participant Scheduler as Scheduler
    participant Dyn as DynamicSchedules
    participant MCP as herdctl-scheduler MCP

    Agent->>FleetMgr: start (self_scheduling enabled)
    FleetMgr->>Scheduler: injectSchedulerMcpServers(agents, stateDir)
    Scheduler->>Dyn: loadAllDynamicSchedules(stateDir)
    Scheduler->>MCP: ensure herdctl-scheduler injected per-agent
    Dyn-->>Scheduler: return dynamic schedules
    Scheduler->>Scheduler: merge static + dynamic schedules
    Scheduler->>Scheduler: evaluate triggers -> create jobs

Loading

sequenceDiagram
    participant CLI as CLI Runtime
    participant Container as Container Runner
    participant Bridge as MCP HTTP Bridge
    participant HostMCP as Host stdio MCP
    participant Agent as Agent process

    CLI->>Container: start execution with injectedMcpServers
    Container->>Bridge: createMcpHostBridge for host servers
    Bridge->>HostMCP: spawn stdio server & connect via SDK client
    HostMCP->>Bridge: listTools / callTool
    Bridge-->>Container: expose HTTP endpoints for tools
    Container->>CLI: pass --mcp-config to CLI
    CLI->>Agent: run prompt (multimodal)
    Agent-->>CLI: return result (tool calls delegated via bridge)

Loading

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~120 minutes

Possibly related PRs

• feat(discord): comprehensive Discord connector improvements #194: Overlapping, large changes across Discord connector, scheduler/self-scheduling, MCP bridging, and runner types — likely touches same files/features.
• feat(discord): add typing_indicator output config option #178: Related changes to Discord output fields (typing_indicator) and attachment handling.
• docs: auto-update documentation (2026-03-13) #205: Documentation overlap updating Discord docs (voice, attachments, output fields).

Suggested reviewers

• edspencer

Poem

🐰
I nibble YAML, clever and spry,
Schedules sprout where carrots lie,
Files tucked neat in rabbit holes,
Bridges hum between the roles,
Agents hop — prompt, run, and reply!

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

[coderabbitai]

Actionable comments posted: 14

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

packages/slack/src/slack-connector.ts (1)

330-355: ⚠️ Potential issue | 🟠 Major

Attachment-only Slack messages still get dropped.

These event.files arguments are only reached after the empty_prompt early return. A DM or mention that contains just a file and no caption is still discarded before buildMessageEvent(), so the new attachment path never runs for that case. Allow attachments to bypass the empty-prompt guard in both handlers.

Possible fix

- const prompt = processMessage(event.text ?? "", this.botUserId);
- if (!prompt) {
+ const prompt = processMessage(event.text ?? "", this.botUserId);
+ const hasAttachments = (event.files?.length ?? 0) > 0;
+ if (!prompt && !hasAttachments) {
    this.messagesIgnored++;
    this.emit("messageIgnored", {
      agentName: this.agentName,
      reason: "empty_prompt",
      channelId: event.channel,
      messageTs: event.ts,
    });
    return;
  }

Apply the same guard change in both the app_mention and message paths.

Also applies to: 461-487

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/slack/src/slack-connector.ts` around lines 330 - 355, The
empty-prompt early return currently drops attachment-only messages; modify the
check around processMessage in both the app_mention and message handlers so that
if prompt is falsy but event.files exists and has length > 0 you do NOT
increment messagesIgnored or emit the "empty_prompt" event and instead continue
to call tryExecuteCommand and buildMessageEvent (so attachment-only DMs/mentions
reach buildMessageEvent with event.files); keep the existing behavior when
prompt is falsy and there are no files. Ensure you update the logic that
references processMessage, tryExecuteCommand, buildMessageEvent, event.files,
and the messagesIgnored/emit("messageIgnored", { reason: "empty_prompt" }) path
in both handlers.

packages/discord/src/__tests__/discord-connector.test.ts (1)

123-133: ⚠️ Potential issue | 🟠 Major

Keep the shared Discord test fixture aligned with the reverted default.

createMockDiscordConfig() now hardcodes result_summary: true, but this PR’s intended default is false. Because this helper backs most connector tests, it makes them exercise the non-default path and can hide regressions in the default behavior. Override it in the specific tests that need summaries instead of baking it into the base fixture.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/discord-connector.test.ts` around lines 123 -
133, The shared test fixture createMockDiscordConfig currently hardcodes
output.result_summary: true which forces tests to use the non-default behavior;
change the fixture to set result_summary to false (or remove the explicit
setting so it defaults to false) and update only the specific tests that require
summaries to override result_summary: true in their test setup. Locate the
createMockDiscordConfig function (and the output object in the test fixture) and
revert/result_summary to the default false, then add explicit overrides in tests
that need summaries rather than baking it into the base fixture.

🟡 Minor comments (15)

.changeset/discord-file-attachments.md-2-11 (1)

2-11: ⚠️ Potential issue | 🟡 Minor

Align this changeset with the actual PR scope.

This reads like a brand-new attachments feature, but the PR is described as a fix release: PDF MIME detection in DMs, result_summary default reverting to false, and warn-level attachment diagnostics. As written, the changelog will mislead users and may also justify the wrong bump level (minor vs likely patch). Please rewrite the entry to describe the fixes shipped here, and include the default/logging changes if they are intended to be user-visible.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.changeset/discord-file-attachments.md around lines 2 - 11, The changeset
currently describes a new attachments feature and marks "@herdctl/core" and
"@herdctl/discord" as minor, but the PR actually fixes PDF MIME detection in
DMs, reverts the default of result_summary to false, and changes attachment
diagnostics to warn-level; update the changeset summary to accurately list those
fixes (PDF MIME detection in DMs, result_summary default=false, attachment
diagnostics now warn) and remove the new-feature wording and any references
implying full attachments support (e.g., remove or revise mentions of
chat.discord.attachments behavior), and change the version bump to patch if no
new public API was added.

.gitignore-50-52 (1)

50-52: ⚠️ Potential issue | 🟡 Minor

Scope the database ignore more narrowly.

*.db is much broader than the Dolt-specific comment here implies. It will ignore any SQLite/test fixture DB anywhere in the repo, which can make legitimate files disappear from git status. Prefer keeping .dolt/ and scoping any DB ignore to a known path or filename instead of a repo-wide glob.

Suggested change

 # Dolt database files (added by bd init)
 .dolt/
-*.db
+# Ignore only known local DB artifacts if needed, e.g.
+# path/to/local-dev.db

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.gitignore around lines 50 - 52, Replace the broad repo-wide glob "*.db"
with a scoped ignore so only Dolt DB files are excluded: keep the existing
".dolt/" entry and remove "*.db", or replace it with a Dolt-scoped pattern such
as ".dolt/*.db" or the explicit Dolt filename used by your tooling (e.g.,
"dolt.db") to avoid accidentally ignoring unrelated SQLite/test DB files.

.beads/backup/issues.jsonl-1-10 (1)

1-10: ⚠️ Potential issue | 🟡 Minor

Issue data file contains PII - verify if appropriate for repository.

Each issue record contains an email address in the owner field (oliver.heckmann@gmail.com). If this repository is public, consider whether storing personal email addresses in committed data files is appropriate from a privacy perspective.

Additionally, like the other backup files, this doesn't implement the PR's stated objectives.

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.beads/backup/issues.jsonl around lines 1 - 10, The JSONL backup contains
personal email addresses in the "owner" field (e.g.,
owner:"oliver.heckmann@gmail.com" in records like id:"herdctl-3ee",
"herdctl-60a", etc.); remove or anonymize these PII values across all records
(replace with a neutral identifier such as owner:null or
owner:"redacted@example.com" or a team handle) and update the commit to avoid
exposing personal emails in the public repo; ensure the change is applied to all
entries in ".beads/backup/issues.jsonl" and add a short commit message
indicating PII sanitization.

packages/core/src/runner/runtime/container-runner.ts-98-103 (1)

98-103: ⚠️ Potential issue | 🟡 Minor

Partial bridge creation failure leaves earlier bridges orphaned.

If createMcpHostBridge throws for a server after previous bridges were successfully created, the earlier bridges in hostBridges array won't be closed because the error will propagate before reaching the finally block that populates hostBridges.

🛡️ Proposed fix: Wrap bridge creation in try-catch for cleanup

       if (Object.keys(hostServers).length > 0) {
         // Start host-side MCP bridges
-        for (const [name, config] of Object.entries(hostServers)) {
-          const handle = await createMcpHostBridge(name, config);
-          hostBridges.push(handle);
+        try {
+          for (const [name, config] of Object.entries(hostServers)) {
+            const handle = await createMcpHostBridge(name, config);
+            hostBridges.push(handle);
+          }
+        } catch (err) {
+          // Clean up any bridges created before the failure
+          for (const handle of hostBridges) {
+            try {
+              await handle.close();
+            } catch {
+              // Ignore cleanup errors
+            }
+          }
+          throw err;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/container-runner.ts` around lines 98 - 103,
The loop that creates MCP host bridges using createMcpHostBridge can throw
partway and leave earlier handles in hostBridges orphaned; wrap each bridge
creation in a try/catch so that on any failure you synchronously/awaitedly clean
up previously created handles by iterating hostBridges and calling their
shutdown/close method (e.g., await handle.close() or the existing cleanup
function used elsewhere), then rethrow the original error; ensure hostBridges is
only pushed to after a successful creation and that the catch/finally path does
the same cleanup to avoid leaking bridges.

AGENTS.md-127-151 (1)

127-151: ⚠️ Potential issue | 🟡 Minor

Remove or rename the duplicate Landing the Plane section.

This repeats the heading and workflow already defined at Lines 15-39, and markdownlint is already flagging MD024 here. Keeping both copies creates two sources of truth.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@AGENTS.md` around lines 127 - 151, The "Landing the Plane (Session
Completion)" section is duplicated and triggers MD024; remove the redundant
block (the heading and the entire workflow currently titled "Landing the Plane
(Session Completion)") or rename and consolidate it into the original canonical
section so there's a single source of truth; update any internal references to
use the remaining section title and ensure the mandatory workflow steps (File
issues, Run quality gates, Update issue status, PUSH TO REMOTE, Clean up,
Verify, Hand off) are preserved in the canonical section.

docs/src/content/docs/integrations/discord.mdx-274-275 (1)

274-275: ⚠️ Potential issue | 🟡 Minor

Fix the documented result_summary default.

The PR reverts output.result_summary back to false, but both the YAML reference and the settings table still say true. That leaves the Discord docs out of sync with the actual default behavior.

📝 Suggested doc fix

-      result_summary: true        # Show completion summary embed (default: true)
+      result_summary: false       # Show completion summary embed (default: false)

-| `output.result_summary` | boolean | `true` | Show a summary embed when the agent finishes (duration, cost, tokens) |
+| `output.result_summary` | boolean | `false` | Show a summary embed when the agent finishes (duration, cost, tokens) |

Also applies to: 372-373

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/src/content/docs/integrations/discord.mdx` around lines 274 - 275,
Update the Discord docs to reflect the actual default of
output.result_summary=false: change the YAML reference entry (the
`result_summary: true` line) and the settings table entries (both occurrences
around the `result_summary`/`errors` blocks) to indicate the default is false,
and ensure any descriptive text referencing `output.result_summary` matches the
reverted default behavior.

packages/discord/src/__tests__/message-normalizer.test.ts-72-90 (1)

72-90: ⚠️ Potential issue | 🟡 Minor

Use distinct SDK and CLI fixtures in the parity test.

sdkResult and cliSyntheticResult are built from the exact same payload, so this only proves the function is deterministic. If the CLI-specific normalization path diverges, this test still passes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/message-normalizer.test.ts` around lines 72 -
90, The test currently constructs sdkResult and cliSyntheticResult from the
identical payload so it only verifies determinism; update the "produces
equivalent result events for SDK and CLI-style result payloads" test to use two
distinct fixtures that represent the SDK and CLI payload shapes (keep sdkResult
and cliSyntheticResult variables and call normalizeDiscordMessage for both),
ensuring the CLI-style payload exercises the CLI-specific normalization path
(e.g., use the CLI field naming/structure that the normalizer expects). Verify
the outputs still equal so the test actually asserts parity between SDK and CLI
inputs for normalizeDiscordMessage.

packages/discord/src/commands/__tests__/command-manager.test.ts-214-217 (1)

214-217: ⚠️ Potential issue | 🟡 Minor

Assert that guild scope replaces global registration.

This only proves that the guild route was hit at least once. If registerCommands() accidentally PUTs both global and guild commands, the test still passes. Add a call-count or negative global-route assertion here.

🧪 Suggested tightening

+      expect(mockRestPut).toHaveBeenCalledTimes(1);
       expect(mockRestPut).toHaveBeenCalledWith(
         expect.stringContaining("/guilds/guild-123/commands"),
         expect.any(Object),
       );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/commands/__tests__/command-manager.test.ts` around lines
214 - 217, The test currently only verifies the guild route was hit once;
tighten it by asserting registerCommands() did not also PUT the global route:
add an assertion on mockRestPut call count (e.g., to ensure only one call) or a
negative assertion that mockRestPut was not called with the global registration
route (string containing the applications/<app-id>/commands/global path), so you
confirm only the guild-specific registration occurred when registerCommands() is
invoked.

packages/core/src/config/__tests__/self-scheduling.test.ts-194-199 (1)

194-199: ⚠️ Potential issue | 🟡 Minor

Make this test actually cover enabled: false.

The test name says “disabled”, but the fixture never sets self_scheduling.enabled to false. That leaves the false branch untested for system prompt injection.

🧪 Proposed fix

   it("does not inject system prompt when self_scheduling is disabled", () => {
-    const agents = [makeAgent()];
+    const agents = [
+      makeAgent({
+        self_scheduling: { enabled: false, max_schedules: 10, min_interval: "5m" },
+      }),
+    ];
 
     injectSchedulerMcpServers(agents, "/tmp/.herdctl");
 
     expect(agents[0].system_prompt).toBeUndefined();

🤖 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 194
- 199, The test "does not inject system prompt when self_scheduling is disabled"
doesn't set self_scheduling.enabled to false, so update the fixture to exercise
the disabled branch: modify the agent produced by makeAgent (or the agent object
passed to injectSchedulerMcpServers) to include self_scheduling: { enabled:
false } before calling injectSchedulerMcpServers; then assert
agents[0].system_prompt is undefined as before. Ensure you reference the
makeAgent call and injectSchedulerMcpServers invocation so the change is applied
to the same agent passed into injectSchedulerMcpServers.

packages/discord/src/message-normalizer.ts-175-182 (1)

175-182: ⚠️ Potential issue | 🟡 Minor

Fall back when the first error string is empty.

If the runtime sends content: "", Lines 176-181 return an empty Discord error even when message has the real text. That leaves the UI with no actionable error details.

🩹 Proposed fix

   if (message.type === "error") {
-    const errorText =
-      typeof (message as { content?: unknown }).content === "string"
-        ? ((message as { content: string }).content ?? "An unknown error occurred")
-        : typeof (message as { message?: unknown }).message === "string"
-          ? ((message as { message: string }).message ?? "An unknown error occurred")
-          : "An unknown error occurred";
+    const contentText =
+      typeof (message as { content?: unknown }).content === "string"
+        ? (message as { content: string }).content.trim()
+        : "";
+    const messageText =
+      typeof (message as { message?: unknown }).message === "string"
+        ? (message as { message: string }).message.trim()
+        : "";
+    const errorText = contentText || messageText || "An unknown error occurred";
     return [{ kind: "error", message: errorText }];
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/message-normalizer.ts` around lines 175 - 182, In the
message.type === "error" branch in message-normalizer.ts, the current errorText
logic treats an empty content string as valid and returns it; change the checks
so you only accept content if it's a non-empty string (e.g., trim() !== ""),
otherwise fall back to message.message if that is a non-empty string, and
finally to the "An unknown error occurred" default; update the variable
errorText calculation accordingly so the UI receives the real error text when
content is empty but message.message contains the error.

packages/discord/src/commands/skills.ts-8-16 (1)

8-16: ⚠️ Potential issue | 🟡 Minor

Differentiate “unsupported” from “no skills found.”

If commandActions.listSkills is absent, Line 9 turns that into an empty list and the command replies “No skills were discovered…”. That hides a missing integration behind a false negative.

🩹 Proposed fix

   async execute(context: CommandContext): Promise<void> {
     const { interaction, commandActions, agentName } = context;
-    const skills = commandActions?.listSkills ? await commandActions.listSkills() : [];
+    if (!commandActions?.listSkills) {
+      await interaction.reply({
+        content: "Skill discovery isn't available for this agent.",
+        ephemeral: true,
+      });
+      return;
+    }
+
+    const skills = await commandActions.listSkills();
 
     if (skills.length === 0) {
       await interaction.reply({
         content: "No skills were discovered for this agent.",

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/commands/skills.ts` around lines 8 - 16, The command
currently treats a missing listSkills integration as an empty result, causing a
misleading "No skills were discovered" reply; update the handler to first check
for the presence of commandActions and commandActions.listSkills (symbols:
commandActions, listSkills) and if absent reply with a clear message that skill
listing is unsupported or the integration is not configured (use
interaction.reply), otherwise call await commandActions.listSkills() and only
then, if the returned skills array is empty, reply with "No skills were
discovered for this agent."

packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts-150-154 (1)

150-154: ⚠️ Potential issue | 🟡 Minor

Read the YAML file here instead of calling readDynamicSchedules() again.

The second read re-applies TTL filtering in memory, so this assertion still passes even if the cleanup was never written back to disk.

🧪 Proposed fix

-      const ondisk = stringifyYaml(
-        JSON.parse(JSON.stringify(await readDynamicSchedules(stateDir, "vector"))),
-      );
+      const ondisk = await readFile(join(dir, "vector.yaml"), "utf-8");
       expect(ondisk).not.toContain("expired");

🤖 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
150 - 154, The test re-reads schedules via readDynamicSchedules(), which applies
in-memory TTL filtering again; instead, read the actual YAML file from disk for
the "vector" tenant and assert its contents do not contain "expired". Replace
the second call to readDynamicSchedules(stateDir, "vector") with a direct file
read (fs.readFile or fs.promises.readFile) of the dynamic schedules YAML file
created for the "vector" tenant, then parse (if needed) and pass the raw file
content into stringifyYaml (or simply use the file string) before running
expect(...).not.toContain("expired"); keep references to stringifyYaml and the
original assertion so the test still verifies on-disk data rather than in-memory
filtering.

packages/discord/src/commands/command-manager.ts-9-9 (1)

9-9: ⚠️ Potential issue | 🟡 Minor

Logger import violates coding guidelines.

The coding guidelines specify that Discord package files should use DiscordLogger from this package for logging, not createLogger from core. The DiscordLogger supports three logging levels (minimal, standard, verbose) with auto-redaction of sensitive fields.

As per coding guidelines: "Use DiscordLogger from this package for logging, not createLogger from core."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/commands/command-manager.ts` at line 9, The file
currently imports createLogger which violates guidelines; replace the import of
createLogger with DiscordLogger from the same package and update usages in
command-manager.ts to instantiate and use DiscordLogger (respecting its logging
levels: minimal, standard, verbose) so logging uses the package's auto-redaction
and level controls; search for createLogger and swap to new DiscordLogger
construction and method calls accordingly.

packages/discord/src/manager.ts-1017-1023 (1)

1017-1023: ⚠️ Potential issue | 🟡 Minor

Mark standalone file replies as user-visible output.

After sending pendingFiles here, the fallback branch still sees sentAnswer === false, hasSentMessages() === false, and embedsSent === 0, so it will also post “no additional output to share.” Set a reply-sent flag when files are delivered, or exclude file-only runs from that fallback.

Also applies to: 1049-1059

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/manager.ts` around lines 1017 - 1023, The fallback posts
“no additional output” after standalone file replies because sending
pendingFiles via event.reply does not flip the reply-sent state; after the await
event.reply({ files: ... }) in the pendingFiles handling block (and the similar
block at 1049-1059) set the same flag the rest of the flow uses to indicate
output was sent (e.g., set sentAnswer = true or call the existing
hasSentMessages/mark-as-sent helper) so the fallback sees that a reply occurred;
alternatively update the condition that triggers the fallback to treat file-only
sends (files.length > 0) as visible output.

packages/core/src/config/schema.ts-636-637 (1)

636-637: ⚠️ Potential issue | 🟡 Minor

Fix the result_summary doc default.

The schema now defaults result_summary to false, but this comment still says true. That will mislead operators reading the config surface.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/schema.ts` around lines 636 - 637, Update the doc
comment for the config field result_summary to reflect the actual default
(false) used in the zod schema: change the comment text that currently reads
"(default: true)" to "(default: false)" so the inline documentation matches the
z.boolean().optional().default(false) declaration in the schema.

🧹 Nitpick comments (15)

packages/core/src/runner/runtime/__tests__/mcp-host-bridge.test.ts (1)

5-31: Consider resetting mocks between tests to prevent test pollution.

The mock functions (mockConnect, mockClose, mockListTools, mockCallTool) are shared across all tests without being reset. This can cause tests to fail or pass incorrectly if run in different orders, since call counts and mock states accumulate.

♻️ Proposed fix: Add mock reset in beforeEach

+import { beforeEach, describe, expect, it, vi } from "vitest";
-import { describe, expect, it, vi } from "vitest";
 import type { McpServer } from "../../../config/index.js";

 // Mock the MCP SDK before importing the module under test
 const mockConnect = vi.fn().mockResolvedValue(undefined);
 const mockClose = vi.fn().mockResolvedValue(undefined);
 const mockListTools = vi.fn().mockResolvedValue({
   // ... existing mock implementation
 });
 const mockCallTool = vi.fn().mockResolvedValue({
   content: [{ type: "text", text: "Note found: Hello World" }],
   isError: false,
 });

+beforeEach(() => {
+  mockConnect.mockClear();
+  mockClose.mockClear();
+  mockListTools.mockClear();
+  mockCallTool.mockClear();
+});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/__tests__/mcp-host-bridge.test.ts` around
lines 5 - 31, The shared mock functions (mockConnect, mockClose, mockListTools,
mockCallTool) accumulate call counts/state across tests; add a beforeEach that
resets their state (e.g., vi.resetAllMocks() or individually call
mockClear/mockReset on mockConnect, mockClose, mockListTools, mockCallTool) so
each test starts with fresh mocks and avoids test pollution when running in
different orders.

packages/discord/src/commands/__tests__/extended-commands.test.ts (1)

94-133: Consider adding more specific assertions for command behavior.

Tests like /new, /stop, /retry only verify that reply was called once, but don't assert on the response content. While integration tests, they could provide more value by checking response structure (e.g., embeds exist, ephemeral flag).

📝 Optional: Add content assertions for basic smoke tests

   it("executes /new", async () => {
     const ctx = makeContext();
     await newCommand.execute(ctx);
-    expect(ctx.interaction.reply).toHaveBeenCalledOnce();
+    expect(ctx.interaction.reply).toHaveBeenCalledOnce();
+    const call = (ctx.interaction.reply as ReturnType<typeof vi.fn>).mock.calls[0][0];
+    expect(call.ephemeral).toBe(true);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/commands/__tests__/extended-commands.test.ts` around
lines 94 - 133, The tests currently only assert that ctx.interaction.reply was
called for commands like newCommand.execute, stopCommand.execute,
retryCommand.execute, skillsCommand.execute, usageCommand.execute,
toolsCommand.execute, configCommand.execute, pingCommand.execute and should be
strengthened to assert reply payload shape and key fields; update each test to
inspect the arguments passed to ctx.interaction.reply (using
toHaveBeenCalledWith/toHaveBeenCalledTimes and argument matchers) and verify
expected properties such as an embed array or content string, presence of
ephemeral: true for ephemeral responses, and any expected fields in embeds
(title/description) for commands like newCommand, skillsCommand and usageCommand
so tests validate response structure rather than only call counts.

packages/core/src/runner/types.ts (1)

43-59: Consider handling empty prompt arrays.

The extractPromptText function handles strings and content block arrays correctly, but an empty array would return an empty string. This may be intentional, but worth documenting if prompts can be empty.

📝 Optional: Add JSDoc note about empty arrays

 /**
  * Extract the text portion of a PromptContent value.
  *
  * When `prompt` is a string, returns it as-is.
  * When `prompt` is ContentBlock[], joins all text blocks and adds
  * "[N image(s) attached]" placeholders for image blocks.
+ *
+ * Note: Returns an empty string for empty ContentBlock arrays.
  */
 export function extractPromptText(prompt: PromptContent): string {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/types.ts` around lines 43 - 59, Add a JSDoc comment
to the extractPromptText function that documents how it handles inputs: state
that when prompt is a string it returns the string, when it's a PromptContent
array it concatenates text blocks and appends a “[N image(s) attached]” marker
for images, and explicitly note the behavior for an empty array (it currently
returns an empty string); update the JSDoc above extractPromptText to make this
contract clear to callers.

packages/discord/src/__tests__/manager.test.ts (2)

176-186: Extract the default Discord output fixture into a helper.

This expanded output object is now copied throughout the file. Every schema/default tweak will require mass edits, and it makes fixture drift easy. A small createDiscordOutput(overrides) helper would keep these tests much easier to maintain.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/manager.test.ts` around lines 176 - 186,
Extract the repeated default Discord output object into a single helper named
createDiscordOutput(overrides) that returns the full default object
(tool_results, tool_result_max_length, system_status, result_summary,
typing_indicator, errors, acknowledge_emoji, assistant_messages,
progress_indicator) merged with any overrides; update all uses in
manager.test.ts to call createDiscordOutput({...}) instead of inlining the
object so tests share one authoritative fixture and can override fields as
needed. Ensure the helper is exported/defined near other test fixtures and used
anywhere the inline output literal currently appears (search for
assistant_messages: "answers" and tool_result_max_length) so future schema
tweaks only require changing the helper.

---

1125-1128: Avoid wall-clock sleeps in these pipeline tests.

These new cases wait 2.5s–5s just to let timing behavior elapse. That will add a lot of runtime to the suite and make CI failures timing-sensitive. Stubbing the delay/rate-limiter would keep the assertions deterministic without burning real time.

Also applies to: 1604-1607, 2374-2377

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/manager.test.ts` around lines 1125 - 1128, The
test uses a real setTimeout to wait for rate-limiting after calling
manager.start() and connector.emit("message", event); instead of sleeping, stub
or mock the rate-limiting/delay so the test runs deterministically: either use
Jest fake timers (jest.useFakeTimers(), then advance timers with
jest.advanceTimersByTime(2500) and restore) around the section that waits, or
mock the rate-limiter/delay implementation used by manager (the module/function
that enforces delays) to resolve immediately; ensure you target the symbols
manager.start, connector.emit("message", ...), and the delay/rate-limiter
function used by the code, and restore real timers/mocks after the test.

packages/discord/src/__tests__/embeds.test.ts (1)

49-57: Potential snapshot flakiness with Date.now() without fake timers.

This test uses Date.now() for startTime (line 51) but doesn't enable fake timers like the other tests. If buildToolResultEmbed calculates duration from startTime, the snapshot may vary slightly between runs due to real time passage during test execution.

♻️ Suggested fix: use fake timers for consistency

   it("builds tool result embed with truncated output", () => {
+    vi.useFakeTimers();
+    vi.setSystemTime(new Date("2026-03-04T12:00:00.000Z"));
     const embed = buildToolResultEmbed({
-      toolUse: { name: "Bash", input: { command: "echo hi" }, startTime: Date.now() - 2000 },
+      toolUse: { name: "Bash", input: { command: "echo hi" }, startTime: Date.now() - 2000 },
       toolResult: { output: "line\n".repeat(200), isError: false },
       agentName: "herdctl.test-agent",
       maxOutputChars: 120,
     });
     expect(embed).toMatchSnapshot();
+    vi.useRealTimers();
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/embeds.test.ts` around lines 49 - 57, The test
for buildToolResultEmbed uses Date.now() for startTime which can make snapshots
flaky; update the test to use Jest fake timers (or otherwise freeze time) around
the call so startTime is deterministic, e.g., call
jest.useFakeTimers()/jest.setSystemTime(...) before creating the embed and
restore timers after; ensure you target the test case that constructs
toolUse.startTime and the buildToolResultEmbed invocation so the
duration-dependent snapshot is stable.

packages/core/src/mcp/scheduler-mcp.ts (1)

49-51: Route startup diagnostics through the core logger.

These console.error calls bypass the shared log-level/filtering behavior used elsewhere in packages/core. This entrypoint should follow the same logging contract as the rest of the core runtime.

♻️ Suggested change

+import { createLogger } from "../utils/logger.js";
+
+const logger = createLogger("scheduler-mcp");
+
 const envResult = EnvSchema.safeParse(process.env);
 if (!envResult.success) {
-  console.error(`Invalid environment: ${envResult.error.issues.map((i) => i.message).join(", ")}`);
+  logger.error(`Invalid environment: ${envResult.error.issues.map((i) => i.message).join(", ")}`);
   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" and "Use relative imports for the logger within @herdctl/core: import { createLogger } from '../utils/logger.js'."

Also applies to: 318-320

🤖 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 49 - 51, Replace raw
console.error usage with the core logger: add "import { createLogger } from
'../utils/logger.js'" at the top, create a logger instance (e.g., const logger =
createLogger('mcp:scheduler') ) and swap console.error(...) calls (the one
checking envResult.success and the other at lines ~318-320) to logger.error(...)
preserving the same message interpolation; keep the process.exit(1) behavior
unchanged. Ensure you reference envResult.error.issues.map(...) in the
logger.error call so startup diagnostics go through the shared createLogger
pipeline.

packages/discord/src/commands/index.ts (1)

36-36: Consider moving usageCommand export for alphabetical consistency.

Other command exports follow alphabetical order. Moving usageCommand between toolsCommand (line 23) and the types block would maintain consistency.

Suggested reordering

 export { stopCommand } from "./stop.js";
 export { toolsCommand } from "./tools.js";
+export { usageCommand } from "./usage.js";
 // Types
 export type {
   ChannelRunUsage,
   CommandActionResult,
   CommandActions,
   CommandContext,
   CommandManagerLogger,
   CommandManagerOptions,
   CumulativeUsage,
   ICommandManager,
   SlashCommand,
 } from "./types.js";
-export { usageCommand } from "./usage.js";

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/commands/index.ts` at line 36, Exports are out of
alphabetical order: move the export for usageCommand so it sits after
toolsCommand and before the types block to match the project's alphabetical
export ordering; locate the export statement "export { usageCommand } from
\"./usage.js\";" and reposition it accordingly alongside other command exports
to maintain consistent ordering.

packages/core/src/fleet-manager/schedule-management.ts (2)

56-61: Type assertion loses some type safety.

The cast to typeof dynamicSchedules discards the full DynamicSchedule type (which includes ttl_hours, created_at, etc.). Since only the common fields are used for building ScheduleInfo, this works but is fragile.

Consider using a mapped type or extracting the schedule-building logic to handle both types uniformly.

🤖 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 56 - 61,
The cast to typeof dynamicSchedules when assigning dynamicSchedules from
loadAllDynamicSchedules loses the full DynamicSchedule type (e.g., ttl_hours,
created_at) and is fragile; instead, update the code to preserve types by either
(a) defining a mapped/union type (e.g., PartialDynamicSchedule |
DynamicSchedule) or (b) extract the schedule conversion into a helper like
buildScheduleInfo(schedule: DynamicSchedule | PartialDynamicSchedule) that
accepts both shapes and returns ScheduleInfo, then call
loadAllDynamicSchedules() without the unsafe assertion and map results through
that helper (referencing dynamicSchedules, loadAllDynamicSchedules,
DynamicSchedule, ScheduleInfo, ttl_hours, created_at).

---

188-221: Redundant agent validation in enableSchedule.

The agent lookup and validation on lines 194-202 duplicates logic that getSchedule() already performs on line 205. The agent name is available from the getSchedule result.

Simplified approach

   async enableSchedule(agentName: string, scheduleName: string): Promise<ScheduleInfo> {
-    const config = this.ctx.getConfig();
     const logger = this.ctx.getLogger();
     const stateDir = this.ctx.getStateDir();
 
-    // Validate the agent exists
-    const agents = config?.agents ?? [];
-    const agent =
-      agents.find((a) => a.qualifiedName === agentName) ?? agents.find((a) => a.name === agentName);
-
-    if (!agent) {
-      throw new AgentNotFoundError(agentName, {
-        availableAgents: agents.map((a) => a.qualifiedName),
-      });
-    }
-
     // Verify the schedule exists (static or dynamic)
-    await this.getSchedule(agent.qualifiedName, scheduleName);
+    const scheduleInfo = await this.getSchedule(agentName, scheduleName);
 
     // Update schedule state to enabled (idle) — use qualifiedName as the state key
     const { updateScheduleState } = await import("../scheduler/schedule-state.js");
     await updateScheduleState(
       stateDir,
-      agent.qualifiedName,
+      scheduleInfo.agentName,
       scheduleName,
       { status: "idle" },
       { logger: { warn: logger.warn } },
     );
 
-    logger.info(`Enabled schedule ${agent.qualifiedName}/${scheduleName}`);
+    logger.info(`Enabled schedule ${scheduleInfo.agentName}/${scheduleName}`);
 
     // Return the updated schedule info
-    return this.getSchedule(agent.qualifiedName, scheduleName);
+    return this.getSchedule(scheduleInfo.agentName, scheduleName);
   }

🤖 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 188 -
221, Remove the duplicated agent lookup in enableSchedule and instead call
getSchedule(...) first to obtain the schedule info (use its agent-qualified name
from the returned ScheduleInfo); replace references to the local agent variable
with the qualifiedName from the getSchedule result when calling
updateScheduleState and when logging, and return that getSchedule result
directly; delete the manual agents array/AgentNotFoundError validation block
since getSchedule already validates the agent. Ensure you still import and call
updateScheduleState with stateDir, the schedule's qualified agent name,
scheduleName, and the same status/logger arguments.

packages/discord/src/discord-connector.ts (1)

325-351: uploadFile implementation looks correct.

The method properly validates client readiness, fetches and validates the channel, and uses AttachmentBuilder for the upload. The fallback to sent.id if no attachment ID exists is a reasonable safety measure.

However, there's no size validation before upload. Discord has file size limits (8MB for regular users, 50MB for Nitro). Consider adding validation to provide a clear error message.

💡 Optional: Add file size validation

   async uploadFile(params: DiscordFileUploadParams): Promise<{ fileId: string }> {
     if (!this._client?.isReady()) {
       throw new Error("Cannot upload file: not connected to Discord");
     }
+
+    // Discord file size limit (8MB for regular, could be higher for Nitro)
+    const MAX_FILE_SIZE = 8 * 1024 * 1024;
+    if (params.fileBuffer.length > MAX_FILE_SIZE) {
+      throw new Error(`File size ${params.fileBuffer.length} exceeds Discord limit of ${MAX_FILE_SIZE} bytes`);
+    }
 
     const channel = await this._client.channels.fetch(params.channelId);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/discord-connector.ts` around lines 325 - 351, Add a
pre-upload size check in uploadFile (accepting DiscordFileUploadParams) before
creating the AttachmentBuilder: inspect params.fileBuffer.length and compare it
to a configurable MAX_FILE_SIZE (or service-specific constants for 8MB/50MB),
and if it exceeds the limit throw a clear Error like "File exceeds Discord
upload limit" (and log via this._logger.error with filename/channelId/size) so
we fail fast rather than attempting the send; keep the existing channel
validation, AttachmentBuilder creation, and return behavior unchanged.

packages/core/src/scheduler/dynamic-schedules.ts (3)

447-447: Dynamic import of readdir could be moved to module-level.

The readdir function is dynamically imported inside loadAllDynamicSchedules. Since fs/promises is already imported at the module level (line 14), consider adding readdir to that import for consistency and marginal performance improvement.

♻️ Move readdir to module-level import

-import { mkdir, readFile, unlink } from "node:fs/promises";
+import { mkdir, readdir, readFile, unlink } from "node:fs/promises";

// ... later in loadAllDynamicSchedules ...

-  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, The dynamic
import of readdir inside loadAllDynamicSchedules should be moved to the
module-level import to match the existing fs/promises usage and avoid a
redundant dynamic import; update the top-of-file import to include readdir
alongside any other fs/promises imports and remove the inline "const { readdir }
= await import('node:fs/promises')" from loadAllDynamicSchedules so the function
uses the module-scoped readdir.

---

71-97: Error classes don't extend FleetManagerError.

Per coding guidelines for core package: "Extend FleetManagerError from fleet-manager/errors.ts for all errors." However, these are scheduler-specific errors rather than fleet-manager errors, so a base DynamicScheduleError extending Error may be intentional for separation of concerns.

🤖 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 71 - 97,
Replace the plain Error base with the shared FleetManagerError: have
DynamicScheduleError extend FleetManagerError (import FleetManagerError from
fleet-manager/errors.ts) and keep the specialized errors
(ScheduleLimitExceededError, MinIntervalViolationError,
ScheduleNameConflictError) extending DynamicScheduleError; ensure constructors
still call super(message) and set this.name appropriately so existing
behavior/identifiers are preserved.

---

506-525: Cron interval validation could fail silently.

The cron gap estimation catches all errors except MinIntervalViolationError, logging failures at debug level. If calculateNextCronTrigger fails consistently (e.g., for edge-case cron expressions), the minimum interval validation is bypassed.

Consider logging at warn level for unexpected failures to aid debugging.

💡 Optional: Log cron estimation failures at warn level

     } catch (error) {
       // Re-throw our own errors, ignore cron parsing failures
       if (error instanceof MinIntervalViolationError) throw error;
-      logger.debug(`Unexpected error estimating cron gap for "${schedule.cron}": ${error}`);
+      logger.warn(`Could not estimate cron gap for "${schedule.cron}": ${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 506 - 525, The
cron gap estimation currently swallows non-MinIntervalViolationError exceptions
by logging them at debug level (in the try/catch around
calculateNextCronTrigger) which can silently bypass min-interval validation;
update the catch to rethrow MinIntervalViolationError as before but change
logger.debug to logger.warn and include contextual info (e.g., scheduleName and
schedule.cron plus the error) so failures in calculateNextCronTrigger are
visible (refer to calculateNextCronTrigger, MinIntervalViolationError, and
logger in the catch block).

packages/core/src/runner/runtime/cli-runtime.ts (1)

304-351: Usage tracking relies on unchecked type assertions.

The trackAssistantUsage function casts message to inline object types without runtime validation. If the message structure differs from expectations (e.g., SDK changes), this could silently produce incorrect usage statistics.

Consider adding defensive checks or using Zod validation for the expected message shape, consistent with the coding guidelines for external input validation.

♻️ Suggested defensive approach

     const trackAssistantUsage = (message: SDKMessage) => {
       if (message.type !== "assistant") {
         return;
       }
 
-      const assistantMeta = message as {
-        message?: {
-          id?: string;
-          stop_reason?: unknown;
-        };
-      };
-      const stopReason = assistantMeta.message?.stop_reason;
+      // Defensively access nested properties
+      const messageObj = "message" in message && typeof message.message === "object" 
+        ? message.message as Record<string, unknown> 
+        : null;
+      if (!messageObj) return;
+      
+      const stopReason = messageObj.stop_reason;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 304 - 351, The
trackAssistantUsage function currently uses unchecked casts of SDKMessage and
assumes fields exist; replace these assertions with runtime validation (either
manual guards or a zod schema) for the expected shape before reading fields:
validate message.type === "assistant", ensure message.message is an object,
message.message.id is a non-empty string, message.message.stop_reason is handled
explicitly, message.message.usage fields are numbers before adding to
totalInputTokens/totalOutputTokens, and message.message.content is an array of
objects with string type/text before constructing lastAssistantText; update uses
of seenAssistantMessageIds, numTurns, totalInputTokens, totalOutputTokens, and
lastAssistantText only after validation and handle missing/invalid values safely
(e.g., treat missing usage as zeros and ignore malformed content).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0e4b490c-6361-44ed-8f7b-8843b168629f

📥 Commits

Reviewing files that changed from the base of the PR and between 7b7ca35 and fb61796.

⛔ Files ignored due to path filters (2)

• packages/discord/src/__tests__/__snapshots__/embeds.test.ts.snap is excluded by !**/*.snap
• pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml

📒 Files selected for processing (86)

• .beads/backup/backup_state.json
• .beads/backup/comments.jsonl
• .beads/backup/config.jsonl
• .beads/backup/dependencies.jsonl
• .beads/backup/events.jsonl
• .beads/backup/issues.jsonl
• .beads/backup/labels.jsonl
• .changeset/agent-self-scheduling.md
• .changeset/cli-runtime-injected-mcp.md
• .changeset/discord-file-attachments.md
• .changeset/discord-pdf-ack-fixes.md
• .gitignore
• AGENTS.md
• docs/src/content/docs/integrations/discord.mdx
• packages/core/package.json
• packages/core/src/config/__tests__/schema.test.ts
• packages/core/src/config/__tests__/self-scheduling.test.ts
• packages/core/src/config/index.ts
• packages/core/src/config/merge.ts
• packages/core/src/config/schema.ts
• packages/core/src/config/self-scheduling.ts
• packages/core/src/fleet-manager/fleet-manager.ts
• packages/core/src/fleet-manager/job-control.ts
• packages/core/src/fleet-manager/schedule-management.ts
• packages/core/src/fleet-manager/status-queries.ts
• packages/core/src/fleet-manager/types.ts
• packages/core/src/mcp/scheduler-mcp.ts
• packages/core/src/runner/index.ts
• packages/core/src/runner/job-executor.ts
• packages/core/src/runner/runtime/__tests__/cli-runtime.test.ts
• packages/core/src/runner/runtime/__tests__/mcp-host-bridge.test.ts
• packages/core/src/runner/runtime/cli-runtime.ts
• packages/core/src/runner/runtime/container-manager.ts
• packages/core/src/runner/runtime/container-runner.ts
• packages/core/src/runner/runtime/docker-config.ts
• packages/core/src/runner/runtime/docker-sdk-wrapper.js
• packages/core/src/runner/runtime/interface.ts
• packages/core/src/runner/runtime/mcp-host-bridge.ts
• packages/core/src/runner/runtime/sdk-runtime.ts
• packages/core/src/runner/types.ts
• packages/core/src/scheduler/__tests__/dynamic-schedules.test.ts
• packages/core/src/scheduler/dynamic-schedules.ts
• packages/core/src/scheduler/index.ts
• packages/core/src/scheduler/scheduler.ts
• packages/discord/README.md
• packages/discord/src/__tests__/attachments.test.ts
• packages/discord/src/__tests__/discord-connector.test.ts
• packages/discord/src/__tests__/embeds.test.ts
• packages/discord/src/__tests__/logger.test.ts
• packages/discord/src/__tests__/manager.test.ts
• packages/discord/src/__tests__/message-normalizer.test.ts
• packages/discord/src/__tests__/runtime-parity.test.ts
• packages/discord/src/auto-mode-handler.ts
• packages/discord/src/commands/__tests__/command-manager.test.ts
• packages/discord/src/commands/__tests__/extended-commands.test.ts
• packages/discord/src/commands/__tests__/help.test.ts
• packages/discord/src/commands/__tests__/reset.test.ts
• packages/discord/src/commands/__tests__/status.test.ts
• packages/discord/src/commands/cancel.ts
• packages/discord/src/commands/command-manager.ts
• packages/discord/src/commands/config.ts
• packages/discord/src/commands/help.ts
• packages/discord/src/commands/index.ts
• packages/discord/src/commands/new.ts
• packages/discord/src/commands/ping.ts
• packages/discord/src/commands/reset.ts
• packages/discord/src/commands/retry.ts
• packages/discord/src/commands/session.ts
• packages/discord/src/commands/skill.ts
• packages/discord/src/commands/skills.ts
• packages/discord/src/commands/status.ts
• packages/discord/src/commands/stop.ts
• packages/discord/src/commands/tools.ts
• packages/discord/src/commands/types.ts
• packages/discord/src/commands/usage.ts
• packages/discord/src/discord-connector.ts
• packages/discord/src/embeds.ts
• packages/discord/src/index.ts
• packages/discord/src/manager.ts
• packages/discord/src/mention-handler.ts
• packages/discord/src/message-normalizer.ts
• packages/discord/src/types.ts
• packages/discord/src/voice-transcriber.ts
• packages/slack/src/manager.ts
• packages/slack/src/slack-connector.ts
• packages/slack/src/types.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Update this changeset to match the actual release surface of the PR.

This entry announces a new @herdctl/core minor for self-scheduling, but the PR objectives here are Discord PDF/default/logging fixes. Merging this as-is will publish the wrong changelog and likely the wrong version bump.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.changeset/agent-self-scheduling.md around lines 2 - 14, The changeset
currently advertises a new "@herdctl/core" minor for agent self-scheduling which
is incorrect for this PR; edit the changeset heading and body so the package
bump reflects the actual release surface (adjust release type to patch/minor as
appropriate), remove the self_scheduling content, and replace it with a concise
summary of the real changes (Discord PDF/default/logging fixes and any affected
packages), ensuring the changed package token "@herdctl/core" and the release
type in the frontmatter match the intended version bump.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Remove the unrelated CLI runtime changeset from this PR.

This entry publishes an @herdctl/core minor for injected MCP server support, which does not match the Discord PDF / result_summary / attachment-diagnostics scope described for PR #200. Leaving it here will generate the wrong release notes and version bump.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.changeset/cli-runtime-injected-mcp.md around lines 1 - 11, The changeset
that lifts "@herdctl/core" to a minor (the injectedMcpServers/CLI runtime entry
referencing mcp-http-bridge and --mcp-config) is unrelated and should be removed
from this PR: delete the changeset file that contains the `"@herdctl/core":
minor` entry (the injected MCP CLI runtime changelog) so the PR only contains
the Discord PDF / result_summary/attachment-diagnostics changes, then re-run the
changeset generator/check and verify no other files (e.g., references to
injectedMcpServers or mcp-http-bridge.ts) were accidentally staged.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Replace same-name MCP servers instead of deep-merging them.

deepMerge() recurses into the server definitions themselves, so an agent cannot actually override a default server with the same name. For example, env: {} still preserves inherited env keys, and a transport switch can leave stale fields behind.

♻️ Proposed fix

-  if (defaults.mcp_servers || agent.mcp_servers) {
-    result.mcp_servers = deepMerge(
-      defaults.mcp_servers as Record<string, unknown> | undefined,
-      agent.mcp_servers as Record<string, unknown> | undefined,
-    ) as AgentConfig["mcp_servers"];
-  }
+  if (defaults.mcp_servers || agent.mcp_servers) {
+    result.mcp_servers = {
+      ...(defaults.mcp_servers ?? {}),
+      ...(agent.mcp_servers ?? {}),
+    };
+  }

📝 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

	// Merge mcp_servers (deep merge — agent servers override defaults with same name,
	// default servers not present in agent config are inherited)
	if (defaults.mcp_servers || agent.mcp_servers) {
	result.mcp_servers = deepMerge(
	defaults.mcp_servers as Record<string, unknown> | undefined,
	agent.mcp_servers as Record<string, unknown> | undefined,
	) as AgentConfig["mcp_servers"];
	if (defaults.mcp_servers || agent.mcp_servers) {
	result.mcp_servers = {
	...(defaults.mcp_servers ?? {}),
	...(agent.mcp_servers ?? {}),
	};
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/merge.ts` around lines 240 - 246, The current merge
in merge.ts uses deepMerge on defaults.mcp_servers and agent.mcp_servers which
recurses into server definitions and prevents an agent from fully replacing a
same-named server; replace that behavior by doing a shallow replace-at-key merge
so any server key present in agent.mcp_servers completely overrides the default
for that key. Edit the block referencing deepMerge, result.mcp_servers,
defaults.mcp_servers, and agent.mcp_servers to build result.mcp_servers by
spreading/assigning defaults first and then agent entries (or iterating keys and
using agent entry when present), ensuring undefined inputs are handled and
removing the deepMerge call for this map-level merge.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Environment variable mutation is not thread-safe.

Modifying process.env directly can cause race conditions if multiple CLI runtime instances execute concurrently in the same process. One job could restore the timeout while another job is still using the extended timeout.

Consider passing the timeout via CLI arguments or environment variables scoped to the subprocess instead of mutating the parent process's environment.

🔧 Suggested approach: Pass timeout to subprocess environment

-      // File uploads via MCP tools can take longer than the default 60s timeout.
-      // Save the original value so we can restore it in `finally` to avoid leaking
-      // state across concurrent jobs.
-      if (options.injectedMcpServers["herdctl-file-sender"]) {
-        if (!process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT) {
-          savedStreamCloseTimeout = undefined; // marker: was not set
-          process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT = "120000";
-        }
-      }
+      // Note: If CLAUDE_CODE_STREAM_CLOSE_TIMEOUT needs to be set for file uploads,
+      // consider passing it through the subprocess spawner's env option instead
+      // of mutating process.env, to avoid race conditions with concurrent jobs.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 267 - 272, The
code mutates process.env.CLAUDE_CODE_STREAM_CLOSE_TIMEOUT inside the CLI runtime
when options.injectedMcpServers["herdctl-file-sender"] is present, which is not
thread-safe; instead, stop changing the global process.env and pass the timeout
only to the subprocess/child invocation or via a CLI argument. Update the logic
in cli-runtime.ts (the block that references
options.injectedMcpServers["herdctl-file-sender"], savedStreamCloseTimeout and
CLAUDE_CODE_STREAM_CLOSE_TIMEOUT) to remove direct global mutation and inject
the "120000" value into the child process spawn/env argument or append a CLI
flag when launching herdctl-file-sender, preserving any savedStreamCloseTimeout
handling locally for that subprocess.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reclaim running persistent containers after a manager restart.

This recovery query only looks for exited/created containers. If herdctl restarts while the previous persistent container is still running, it won't be found here and a second container gets created for the same agent. Include running in the search and short-circuit reuse before calling start().

Suggested direction

       const existing = await this.docker.listContainers({
         all: true,
         limit: 1,
         filters: {
           name: [`herdctl-${agentName}-`],
-          status: ["exited", "created"],
+          status: ["running", "exited", "created"],
         },
       });

       if (existing.length > 0) {
         const container = this.docker.getContainer(existing[0].Id);
         try {
-          await container.start();
+          const info = await container.inspect();
+          if (!info.State.Running) {
+            await container.start();
+          }
           this.runningContainers.set(agentName, container);
-          logger.info(`Restarted existing container for ${agentName}: ${existing[0].Names[0]}`);
+          logger.info(`Reused existing container for ${agentName}: ${existing[0].Names[0]}`);
           return container;
         } catch (err) {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/container-manager.ts` around lines 63 - 85,
The recovery query in container-manager.ts (this.docker.listContainers) only
filters for "exited" and "created" so a still-running persistent container is
missed; update the filters to include "running" and, when you find an existing
container whose status is "running", short-circuit: call
this.runningContainers.set(agentName, container) and return it without calling
container.start(); for non-running matches keep the existing start()/catch flow.
Ensure you reference the same agentName, container, this.docker.listContainers,
container.start, and this.runningContainers.set symbols when making the change.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Reserve the channel before kicking off the background run.

activeJobsByChannel is only populated later in onJobCreated(). Two slash-triggered runs can both pass the guard and start separate jobs in the same channel before that callback fires. Put a pending sentinel/lock in place before handleMessage() starts and replace it with the real job id once it exists.

Also applies to: 1456-1464, 1499-1512

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/manager.ts` around lines 764 - 769, Race can occur
because activeJobsByChannel is only set in onJobCreated; before starting
handleMessage we must reserve the channel with a pending sentinel so concurrent
handlers fail the guard. Update the flow where handleMessage is invoked to set
this.activeJobsByChannel.set(this.getChannelKey(qualifiedName,
event.metadata.channelId), PENDING_SENTINEL) immediately before launching the
background job, and then in onJobCreated replace the PENDING_SENTINEL with the
real jobId (or clear it on error). Use a unique constant like PENDING_SENTINEL
and ensure the guard that prevents new runs treats the sentinel as an active
lock; apply the same change at the other occurrences referenced (around the
blocks for lines ~1456-1464 and ~1499-1512).

[coderabbitai]

⚠️ Potential issue | 🟠 Major

tool_result_max_length is ignored for normal tool outputs.

This branch only emits a tool-result reply when the output is larger than the limit. For anything under the threshold, users only get the hard-coded 90-character trace preview, so tool_results: true no longer shows the configured amount of output. Emit a normal buildToolResultEmbed(...) in the non-oversize path too.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/manager.ts` around lines 869 - 905, The code only sends
a full tool result embed when output exceeds
outputConfig.tool_result_max_length, leaving small outputs as a 90-char trace
preview; modify the branch handling normalized.kind === "tool_results" in
manager.ts so that for non-oversized outputs you also build and send the full
embed via buildToolResultEmbed (using toolUse, toolResult, agentName:
qualifiedName and maxOutputChars from outputConfig.tool_result_max_length) and
call event.reply with embeds (no file), while preserving the existing
oversized-path behavior (flush streamer, attach file, increment embedsSent) and
still updating pushTraceLine/refreshRunCard/latestStatusText as currently done.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Apply the same embed-text extraction to the trigger message.

This only enriches processMessage(). buildConversationContext() still builds prompt from triggerMessage.content alone, so an embed-only mention/link preview will arrive with an empty prompt even though history messages now get the richer text. Please pull this into a shared helper and use it for both history messages and the trigger prompt path.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/mention-handler.ts` around lines 234 - 257, The embed
extraction logic used in processMessage() should be refactored into a shared
helper (e.g., extractEmbedTextFromMessage(message): string) and then used in
buildConversationContext() when building the trigger prompt so triggerMessage
isn't limited to triggerMessage.content; update buildConversationContext() to
call the new helper and append/embed its result into the prompt the same way
processMessage() does (including the 4000-char cap and truncation marker) so
both history messages and the trigger message receive identical rich-text
extraction.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and view the relevant section
fd voice-transcriber.ts packages/discord/src

Repository: edspencer/herdctl

Length of output: 102

---

🏁 Script executed:

# Check the current implementation around lines 61-65
cat -n packages/discord/src/voice-transcriber.ts | sed -n '50,75p'

Repository: edspencer/herdctl

Length of output: 771

---

🏁 Script executed:

# Search for Zod imports in the discord package to understand existing patterns
rg "import.*zod" packages/discord/src/ -i

Repository: edspencer/herdctl

Length of output: 43

---

🏁 Script executed:

# Check if Zod is used for validation elsewhere in discord package
rg "z\.(object|string|parse)" packages/discord/src/ -A 2 -B 2

Repository: edspencer/herdctl

Length of output: 43

---

🏁 Script executed:

# Check the imports at the top of voice-transcriber.ts
head -30 packages/discord/src/voice-transcriber.ts

Repository: edspencer/herdctl

Length of output: 971

---

🏁 Script executed:

# Check for error handling patterns in the file and error classes used
rg "throw new Error|ConnectorError" packages/discord/src/voice-transcriber.ts -A 1 -B 1

Repository: edspencer/herdctl

Length of output: 368

---

🏁 Script executed:

# Check if Zod is in the dependencies
grep -r "zod" package.json packages/discord/package.json 2>/dev/null

Repository: edspencer/herdctl

Length of output: 43

---

🏁 Script executed:

# Check for Zod usage across the entire codebase to see if it's established
rg "from \"zod\"|from 'zod'" --max-count 5

Repository: edspencer/herdctl

Length of output: 1194

---

🏁 Script executed:

# Check what error classes or patterns are used in the discord package for external API calls
rg "class.*Error|ConnectorError" packages/discord/src/ -B 2 -A 5

Repository: edspencer/herdctl

Length of output: 39044

---

🏁 Script executed:

# Check full imports section of voice-transcriber.ts to see what's already there
head -50 packages/discord/src/voice-transcriber.ts

Repository: edspencer/herdctl

Length of output: 1585

---

🏁 Script executed:

# Check if Zod is used anywhere in the discord package
rg "from.*zod|import.*z" packages/discord/src/

Repository: edspencer/herdctl

Length of output: 407

---

🏁 Script executed:

# Check the module documentation comment more carefully
sed -n '1,10p' packages/discord/src/voice-transcriber.ts

Repository: edspencer/herdctl

Length of output: 366

---

🏁 Script executed:

# Check package.json to see if zod is a dependency in discord package
cat packages/discord/package.json | grep -A 20 -B 5 "dependencies"

Repository: edspencer/herdctl

Length of output: 671

---

🏁 Script executed:

# Search for how errors are thrown in voice-transcriber.ts
rg "throw" packages/discord/src/voice-transcriber.ts -B 2 -A 1

Repository: edspencer/herdctl

Length of output: 622

---

🏁 Script executed:

# Check if zod is in the root/workspace level
cat package.json | grep -i zod

Repository: edspencer/herdctl

Length of output: 43

---

🏁 Script executed:

# Check the entire voice-transcriber.ts file to understand full context
wc -l packages/discord/src/voice-transcriber.ts

Repository: edspencer/herdctl

Length of output: 105

---

🏁 Script executed:

# View the complete file to understand all error patterns
cat -n packages/discord/src/voice-transcriber.ts

Repository: edspencer/herdctl

Length of output: 2590

---

Use a Zod schema for OpenAI Whisper API response validation.

This endpoint returns external JSON that must be validated. Currently the code uses a manual typeof check, but a Zod schema aligns with project standards and provides cleaner error handling if the API response shape changes. Zod is available via @herdctl/core, which is already a dependency.

Suggested schema-based parse

+import { z } from "zod";
+
+const TranscriptionResponseSchema = z.object({
+  text: z.string(),
+});
+
 export async function transcribeAudio(
   audioBuffer: Buffer,
   filename: string,
   options: TranscribeOptions,
 ): Promise<TranscribeResult> {
   // ... API call setup ...
 
   if (!response.ok) {
     const errorBody = await response.text().catch(() => "unknown error");
     throw new Error(`OpenAI Whisper API error (${response.status}): ${errorBody}`);
   }
 
-  const result = (await response.json()) as Record<string, unknown>;
-  if (typeof result.text !== "string") {
-    throw new Error("OpenAI Whisper API returned an unexpected response (missing 'text' field)");
-  }
-  return { text: result.text };
+  const result = TranscriptionResponseSchema.parse(await response.json());
+  return { text: result.text };
 }

📝 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

	const result = (await response.json()) as Record<string, unknown>;
	if (typeof result.text !== "string") {
	throw new Error("OpenAI Whisper API returned an unexpected response (missing 'text' field)");
	}
	return { text: result.text };
	import { z } from "zod";
	const TranscriptionResponseSchema = z.object({
	text: z.string(),
	});
	export async function transcribeAudio(
	audioBuffer: Buffer,
	filename: string,
	options: TranscribeOptions,
	): Promise<TranscribeResult> {
	// ... API call setup ...
	if (!response.ok) {
	const errorBody = await response.text().catch(() => "unknown error");
	throw new Error(`OpenAI Whisper API error (${response.status}): ${errorBody}`);
	}
	const result = TranscriptionResponseSchema.parse(await response.json());
	return { text: result.text };
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/voice-transcriber.ts` around lines 61 - 65, Replace the
manual typeof check on the Whisper response with a Zod schema parse: import z
from '@herdctl/core', define a schema like z.object({ text: z.string() }) and
use schema.parse(result) (or safeParse and throw a clear error) where the
current `result` is validated; update the function in voice-transcriber.ts that
reads `const result = (await response.json()) as Record<string, unknown>;` to
parse via Zod and return the parsed `.text` value (preserving the same return
shape) so any schema mismatch produces a descriptive validation error.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Add a hard size cap before buffering Slack images.

This path downloads every image fully into memory and then base64-encodes it, but attachment.size is never checked. A large Slack upload can spike memory on this path and still fail downstream on prompt-size limits. Skip or downsize attachments over a defined byte budget before calling arrayBuffer().

Possible fix

+ private static readonly MAX_IMAGE_BYTES = 5 * 1024 * 1024;
+
   for (const attachment of imageAttachments) {
+    if (attachment.size > SlackManager.MAX_IMAGE_BYTES) {
+      logger.warn(
+        `Skipping Slack image ${attachment.name}: ${attachment.size} bytes exceeds ${SlackManager.MAX_IMAGE_BYTES} byte limit`,
+      );
+      continue;
+    }
+
     try {
       // Slack url_private requires Bearer token authentication
       const response = await fetch(attachment.urlPrivate, {
         headers: { Authorization: `Bearer ${botToken}` },
         signal: AbortSignal.timeout(30_000),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/slack/src/manager.ts` around lines 794 - 845, In
buildPromptWithAttachments (SlackManager) add a hard byte-size check before
downloading/ buffering: define a MAX_ATTACHMENT_BYTES constant, inspect
attachment.size (or use response.headers.get('content-length') / response.body
stream to enforce the cap) and if the file exceeds the cap, log a warning via
logger.warn and skip the attachment; for files slightly over the limit consider
streaming/downsizing or rejecting earlier rather than calling
response.arrayBuffer() so you never allocate large buffers in memory and
downstream prompt-size limits are respected.

[coderabbitai]

Actionable comments posted: 5

♻️ Duplicate comments (1)

packages/core/src/runner/runtime/cli-runtime.ts (1)

267-271: ⚠️ Potential issue | 🟠 Major

Per-run timeout should not be written into shared process.env.

Concurrent CLIRuntime executions can clobber each other here and restore the wrong timeout value afterward. This still needs to move into the spawned subprocess environment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 267 - 271, The
code writes per-run timeout into the global process.env
(CLAUDE_CODE_STREAM_CLOSE_TIMEOUT) inside CLIRuntime, which can be clobbered by
concurrent runs; instead avoid mutating process.env and pass the timeout only
into the spawned subprocess environment: when
options.injectedMcpServers["herdctl-file-sender"] is true compute the desired
timeout value locally (use savedStreamCloseTimeout as an internal marker
variable but do not set process.env), then when creating the child process
(where spawn/exec is performed) merge process.env with {
CLAUDE_CODE_STREAM_CLOSE_TIMEOUT: "120000" } in the child options.env so only
the child sees the override; remove any logic that restores process.env from
savedStreamCloseTimeout.

🧹 Nitpick comments (3)

packages/core/src/config/self-scheduling.ts (2)

64-68: Consider adding debug logging for injection operations.

For operational visibility, debug-level logging when MCP servers are injected would help operators troubleshoot self-scheduling configuration. Per coding guidelines, use createLogger from the appropriate relative path.

♻️ Suggested addition

+import { createLogger } from "../utils/logger.js";
+
+const logger = createLogger("self-scheduling");
+
 export function injectSchedulerMcpServers(agents: ResolvedAgent[], stateDir: string): void {
   const mcpPath = getSchedulerMcpPath();

   for (const agent of agents) {
     if (!agent.self_scheduling?.enabled) continue;

+    logger.debug(`Injecting scheduler MCP server for agent: ${agent.qualifiedName}`);
+
     const selfScheduling = agent.self_scheduling;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/self-scheduling.ts` around lines 64 - 68, Add
debug-level logging to injectSchedulerMcpServers to record when MCP servers are
injected and which agent is affected: import and instantiate createLogger, call
it (e.g., logger.debug) before and after injection steps inside
injectSchedulerMcpServers and include the mcpPath and agent.id or agent.name
plus whether agent.self_scheduling?.enabled was true; use getSchedulerMcpPath to
show the path in the log and ensure messages are concise and only logged at
debug level.

---

34-35: Consider extracting default values as shared constants.

The defaults 10 and "5m" are duplicated here and at lines 88-89 when constructing env vars. They also match the defaults in scheduler-mcp.ts EnvSchema. Extracting these as module-level constants would prevent drift if limits are adjusted later.

♻️ Suggested refactor

+const DEFAULT_MAX_SCHEDULES = 10;
+const DEFAULT_MIN_INTERVAL = "5m";
+
 function buildSchedulingPrompt(
   selfScheduling: NonNullable<ResolvedAgent["self_scheduling"]>,
 ): string {
-  const maxSchedules = selfScheduling.max_schedules ?? 10;
-  const minInterval = selfScheduling.min_interval ?? "5m";
+  const maxSchedules = selfScheduling.max_schedules ?? DEFAULT_MAX_SCHEDULES;
+  const minInterval = selfScheduling.min_interval ?? DEFAULT_MIN_INTERVAL;

Then use the same constants at lines 88-89.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/self-scheduling.ts` around lines 34 - 35, Extract
the duplicated defaults into module-level constants (e.g., DEFAULT_MAX_SCHEDULES
= 10 and DEFAULT_MIN_INTERVAL = "5m") and replace the inline literals used when
reading selfScheduling (maxSchedules = selfScheduling.max_schedules ?? ...) and
when constructing env vars later in this module with those constants; ensure the
same constants align with the EnvSchema defaults in scheduler-mcp.ts so updates
are centralized and avoid drift.

packages/core/src/runner/types.ts (1)

15-28: Use interface for the block object shapes.

TextBlock and ImageBlock are plain object shapes, so they should follow the repo convention and be declared as interfaces. The union aliases can stay as types.

♻️ Proposed refactor

-export type TextBlock = { type: "text"; text: string };
+export interface TextBlock {
+  type: "text";
+  text: string;
+}

-export type ImageBlock = {
+export interface ImageBlock {
   type: "image";
   source: {
     type: "base64";
     media_type: "image/jpeg" | "image/png" | "image/gif" | "image/webp";
     data: string; // base64-encoded
   };
-};
+}

As per coding guidelines, "Prefer interface over type for defining object shapes in TypeScript".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/types.ts` around lines 15 - 28, Convert the
object-shaped aliases TextBlock and ImageBlock from type declarations to
exported interfaces keeping their exact property shapes and JSDoc comments
(interface TextBlock { type: "text"; text: string } and interface ImageBlock {
type: "image"; source: { type: "base64"; media_type: "image/jpeg" | "image/png"
| "image/gif" | "image/webp"; data: string; }; };), and leave the union alias
ContentBlock as a type alias (export type ContentBlock = TextBlock | ImageBlock)
so external usage and discriminated unions remain unchanged; ensure all exports
and names (TextBlock, ImageBlock, ContentBlock) match the originals.

🤖 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/runner/runtime/cli-runtime.ts`:
- Around line 439-455: The race between sessionFilePromise and
processExitPromise can falsely choose process_done before a late .jsonl appears;
update the fallback in the Promise.race handling so that when raceResult.type
=== "process_done" you wait a short grace period (e.g., 50–200ms) or attempt a
final non-blocking check for the session file before generating a synthetic ID.
Concretely, after detecting process_done (in the same block that currently
computes syntheticId and sets sessionFilePath), re-check the session directory
for the expected file path (or re-await/inspect sessionFilePromise with a short
timeout) and only create the synthetic path if the file truly does not exist;
use the existing symbols sessionFilePromise, processExitPromise, raceResult,
sessionFilePath, sessionDir, and capturedSessionId to locate and modify the
logic.
- Around line 384-387: The code is currently logging raw Claude stdout at info
level inside the subprocess.stdout?.on("data", ...) handler (variable text and
logger.info), which can leak model/user data; change this to not emit the raw
chunk at info—either remove logger.info(text) or demote it to logger.debug(text)
— then parse the stream to extract session_id and log only the extracted
session_id (or other minimal metadata) at info level; ensure the parsing and
session_id extraction logic remains in the same handler so no raw content is
emitted to default logs.
- Around line 161-170: The fallback branch currently pushes
options.systemPromptAppend using "--system-prompt", which replaces the default
Claude Code prompt; change that branch to push it with "--append-system-prompt"
(i.e., update the args.push call where only options.systemPromptAppend is
present so it uses "--append-system-prompt" and options.systemPromptAppend
instead of "--system-prompt"). This ensures that when
options.agent.system_prompt is absent but options.systemPromptAppend exists, the
code appends to the default prompt rather than replacing it; locate the
conditional handling options.agent.system_prompt and options.systemPromptAppend
and modify the final args.push accordingly.

In `@packages/core/src/runner/types.ts`:
- Around line 48-57: The loop that builds parts currently collects image blocks
by incrementing imageCount and appending a combined "[{n} image(s) attached]" at
the end, which reorders images; change the loop so when block.type === "image"
you push the image placeholder into parts at that position (e.g. push the same
placeholder string used today, such as "[1 image(s) attached]" or a single-image
variant) instead of incrementing imageCount, and remove the final if (imageCount
> 0) append; update any tests that expect the old suffix behavior. Ensure you
edit the code handling prompt, parts and imageCount in this file
(packages/core/src/runner/types.ts) to push placeholders inline rather than
aggregating them.

In `@packages/core/src/state/session-validation.ts`:
- Around line 144-146: The current existence helper returns stats.size > 0 which
treats 0-byte Docker session files as missing; change it to return true on a
successful stat (i.e., treat any present file, including size === 0, as
existing) so validateSessionWithFileCheck() doesn't mark transient, just-created
session files as file_not_found; if you need to reject permanently empty files,
implement a separate "resume-ready" check (e.g., isSessionResumeReady or
similar) that explicitly verifies stats.size > 0 after the writer/append phase.

---

Duplicate comments:
In `@packages/core/src/runner/runtime/cli-runtime.ts`:
- Around line 267-271: The code writes per-run timeout into the global
process.env (CLAUDE_CODE_STREAM_CLOSE_TIMEOUT) inside CLIRuntime, which can be
clobbered by concurrent runs; instead avoid mutating process.env and pass the
timeout only into the spawned subprocess environment: when
options.injectedMcpServers["herdctl-file-sender"] is true compute the desired
timeout value locally (use savedStreamCloseTimeout as an internal marker
variable but do not set process.env), then when creating the child process
(where spawn/exec is performed) merge process.env with {
CLAUDE_CODE_STREAM_CLOSE_TIMEOUT: "120000" } in the child options.env so only
the child sees the override; remove any logic that restores process.env from
savedStreamCloseTimeout.

---

Nitpick comments:
In `@packages/core/src/config/self-scheduling.ts`:
- Around line 64-68: Add debug-level logging to injectSchedulerMcpServers to
record when MCP servers are injected and which agent is affected: import and
instantiate createLogger, call it (e.g., logger.debug) before and after
injection steps inside injectSchedulerMcpServers and include the mcpPath and
agent.id or agent.name plus whether agent.self_scheduling?.enabled was true; use
getSchedulerMcpPath to show the path in the log and ensure messages are concise
and only logged at debug level.
- Around line 34-35: Extract the duplicated defaults into module-level constants
(e.g., DEFAULT_MAX_SCHEDULES = 10 and DEFAULT_MIN_INTERVAL = "5m") and replace
the inline literals used when reading selfScheduling (maxSchedules =
selfScheduling.max_schedules ?? ...) and when constructing env vars later in
this module with those constants; ensure the same constants align with the
EnvSchema defaults in scheduler-mcp.ts so updates are centralized and avoid
drift.

In `@packages/core/src/runner/types.ts`:
- Around line 15-28: Convert the object-shaped aliases TextBlock and ImageBlock
from type declarations to exported interfaces keeping their exact property
shapes and JSDoc comments (interface TextBlock { type: "text"; text: string }
and interface ImageBlock { type: "image"; source: { type: "base64"; media_type:
"image/jpeg" | "image/png" | "image/gif" | "image/webp"; data: string; }; };),
and leave the union alias ContentBlock as a type alias (export type ContentBlock
= TextBlock | ImageBlock) so external usage and discriminated unions remain
unchanged; ensure all exports and names (TextBlock, ImageBlock, ContentBlock)
match the originals.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 84ecc738-207f-4368-8071-09b3a1d8ddfe

📥 Commits

Reviewing files that changed from the base of the PR and between fb61796 and 069b82c.

📒 Files selected for processing (6)

• packages/core/src/config/__tests__/self-scheduling.test.ts
• packages/core/src/config/self-scheduling.ts
• packages/core/src/runner/runtime/cli-runtime.ts
• packages/core/src/runner/sdk-adapter.ts
• packages/core/src/runner/types.ts
• packages/core/src/state/session-validation.ts

🚧 Files skipped from review as they are similar to previous changes (1)

• packages/core/src/config/tests/self-scheduling.test.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

Does the Claude CLI --system-prompt option replace the default Claude Code system prompt, or is there an official way to append text to the default/preset system prompt? Please check official Anthropic/Claude Code documentation.

💡 Result:

Yes—per the official Claude Code CLI reference, --system-prompt replaces the entire default Claude Code system prompt (“removes all default Claude Code instructions”). [1]

To append text while keeping the built-in/default prompt, the official mechanism is to use --append-system-prompt (or --append-system-prompt-file). [1] Anthropic also notes in the official settings docs that Claude Code’s internal system prompt isn’t published, and directs users to CLAUDE.md or --append-system-prompt to add custom instructions. [2]

Sources: [1] CLI reference (Claude Code) • [2] Claude Code settings docs

---

Use --append-system-prompt to append text to the default Claude Code system prompt.

The Claude Code CLI option --system-prompt replaces the entire default Claude Code system prompt; to append text while preserving the default, use --append-system-prompt or --append-system-prompt-file.

The fallback branch (lines 165–167) treats systemPromptAppend as the sole system prompt, passing it via --system-prompt. This replaces the default Claude Code prompt instead of appending to it, breaking the append semantics specified in RunnerOptions.

Update the fallback case to pass systemPromptAppend via --append-system-prompt instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 161 - 170, The
fallback branch currently pushes options.systemPromptAppend using
"--system-prompt", which replaces the default Claude Code prompt; change that
branch to push it with "--append-system-prompt" (i.e., update the args.push call
where only options.systemPromptAppend is present so it uses
"--append-system-prompt" and options.systemPromptAppend instead of
"--system-prompt"). This ensures that when options.agent.system_prompt is absent
but options.systemPromptAppend exists, the code appends to the default prompt
rather than replacing it; locate the conditional handling
options.agent.system_prompt and options.systemPromptAppend and modify the final
args.push accordingly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don’t log raw Claude stdout at info.

These chunks can contain full model output and user data, so this now leaks normal-run content into default logs. Parse the session_id without emitting the raw chunk, or move any dump behind debug.

As per coding guidelines, "Choose appropriate log levels: debug for internal details, info for significant events, warn for recoverable issues, error for failures".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 384 - 387, The
code is currently logging raw Claude stdout at info level inside the
subprocess.stdout?.on("data", ...) handler (variable text and logger.info),
which can leak model/user data; change this to not emit the raw chunk at
info—either remove logger.info(text) or demote it to logger.debug(text) — then
parse the stream to extract session_id and log only the extracted session_id (or
other minimal metadata) at info level; ensure the parsing and session_id
extraction logic remains in the same handler so no raw content is emitted to
default logs.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Wait briefly after process exit before falling back to a synthetic session path.

A real .jsonl can appear a moment after the CLI exits, especially on Docker or mounted filesystems. Once process_done wins this race, the runtime never watches the real file, so late final messages are lost and resume is disabled even though the session file may arrive milliseconds later.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/runtime/cli-runtime.ts` around lines 439 - 455, The
race between sessionFilePromise and processExitPromise can falsely choose
process_done before a late .jsonl appears; update the fallback in the
Promise.race handling so that when raceResult.type === "process_done" you wait a
short grace period (e.g., 50–200ms) or attempt a final non-blocking check for
the session file before generating a synthetic ID. Concretely, after detecting
process_done (in the same block that currently computes syntheticId and sets
sessionFilePath), re-check the session directory for the expected file path (or
re-await/inspect sessionFilePromise with a short timeout) and only create the
synthetic path if the file truly does not exist; use the existing symbols
sessionFilePromise, processExitPromise, raceResult, sessionFilePath, sessionDir,
and capturedSessionId to locate and modify the logic.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Preserve image placeholders in the original block order.

This currently moves all image markers to the end, so an interleaved prompt like text -> image -> text becomes text -> text -> [1 image(s) attached]. That changes the CLI fallback prompt semantics for references like “in the image above/below”.

🛠️ Proposed fix

   const parts: string[] = [];
-  let imageCount = 0;
   for (const block of prompt) {
     if (block.type === "text") {
       parts.push(block.text);
     } else if (block.type === "image") {
-      imageCount++;
+      parts.push("[image attached]");
     }
   }
-  if (imageCount > 0) {
-    parts.push(`[${imageCount} image(s) attached]`);
-  }
   return parts.join("\n");

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/runner/types.ts` around lines 48 - 57, The loop that builds
parts currently collects image blocks by incrementing imageCount and appending a
combined "[{n} image(s) attached]" at the end, which reorders images; change the
loop so when block.type === "image" you push the image placeholder into parts at
that position (e.g. push the same placeholder string used today, such as "[1
image(s) attached]" or a single-image variant) instead of incrementing
imageCount, and remove the final if (imageCount > 0) append; update any tests
that expect the old suffix behavior. Ensure you edit the code handling prompt,
parts and imageCount in this file (packages/core/src/runner/types.ts) to push
placeholders inline rather than aggregating them.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't treat a newly created 0-byte Docker session file as missing.

This helper is named and documented as an existence check, but stats.size > 0 turns it into a content-readiness check. packages/core/src/runner/runtime/cli-runtime.ts:454-485 already documents that the Docker session path can be observed before the first append, so size === 0 is a valid transient state. Returning false here makes validateSessionWithFileCheck() mark an initializing session as file_not_found, which can trigger duplicate session creation.

Suggested fix

-    const stats = await stat(sessionFile);
-    // Empty files (0 bytes) are invalid — they can't be resumed
-    return stats.size > 0;
+    const stats = await stat(sessionFile);
+    return stats.isFile();

If you still want to reject permanently empty files, do that in a separate "resume-ready" check after the writer has had a chance to populate the file.

📝 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

	const stats = await stat(sessionFile);
	// Empty files (0 bytes) are invalid — they can't be resumed
	return stats.size > 0;
	const stats = await stat(sessionFile);
	return stats.isFile();

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/state/session-validation.ts` around lines 144 - 146, The
current existence helper returns stats.size > 0 which treats 0-byte Docker
session files as missing; change it to return true on a successful stat (i.e.,
treat any present file, including size === 0, as existing) so
validateSessionWithFileCheck() doesn't mark transient, just-created session
files as file_not_found; if you need to reject permanently empty files,
implement a separate "resume-ready" check (e.g., isSessionResumeReady or
similar) that explicitly verifies stats.size > 0 after the writer/append phase.

[coderabbitai]

Actionable comments posted: 4

🧹 Nitpick comments (4)

packages/slack/src/__tests__/manager.test.ts (1)

73-73: Optional: deduplicate the repeated self_scheduling fixture literal.

Same object is defined twice; a shared constant would make future default changes safer.

Suggested refactor

+const defaultSelfScheduling = { enabled: true, max_schedules: 10, min_interval: "5m" } as const;
+
 function createSlackAgent(name: string, slackConfig: AgentChatSlack): ResolvedAgent {
   return {
@@
-    self_scheduling: { enabled: true, max_schedules: 10, min_interval: "5m" },
+    self_scheduling: { ...defaultSelfScheduling },
   } as ResolvedAgent;
 }
@@
 function createNonSlackAgent(name: string): ResolvedAgent {
   return {
@@
-    self_scheduling: { enabled: true, max_schedules: 10, min_interval: "5m" },
+    self_scheduling: { ...defaultSelfScheduling },
   } as ResolvedAgent;
 }

Also applies to: 85-85

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/slack/src/__tests__/manager.test.ts` at line 73, Extract the
repeated self_scheduling literal into a single shared constant (e.g., const
SELF_SCHEDULING = { enabled: true, max_schedules: 10, min_interval: "5m" }) and
replace the duplicate inline objects in the test file with references to that
constant; update occurrences used in the tests that currently define
self_scheduling inline (the fixtures around the manager tests) to use
SELF_SCHEDULING so future changes only require editing one place.

packages/discord/src/__tests__/manager.test.ts (2)

580-582: Remove console.log from mock function.

The mock reply function logs to console, which can clutter test output. While this is test code, it's better practice to use a no-op or track calls via the mock itself.

♻️ Suggested fix

-      reply: async (content) => {
-        console.log("Reply:", content);
-      },
+      reply: vi.fn().mockResolvedValue(undefined),

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/manager.test.ts` around lines 580 - 582, The
test mock for the reply function currently prints to stdout; replace the inline
console.log in the mock reply implementation with a no-op or a jest mock so
tests don't clutter output—e.g., change the reply mock (reply: async (content)
=> { console.log("Reply:", content); }) to a silent implementation or use
jest.fn()/sinon stub to record calls (reply:
jest.fn().mockResolvedValue(undefined)) so assertions can still inspect calls
without printing.

---

1086-1086: Consider reducing hardcoded timeout delays.

Several tests use setTimeout with 2500ms delays to "wait for rate limiting". These long delays can make tests slow and potentially flaky in CI environments.

Consider using shorter delays (e.g., 50-100ms) for tests where rate limiting isn't the focus, or use vi.useFakeTimers() to control time advancement deterministically:

// Example using fake timers
beforeEach(() => {
  vi.useFakeTimers();
});

afterEach(() => {
  vi.useRealTimers();
});

// In test:
connector.emit("message", event);
await vi.runAllTimersAsync();

Also applies to: 1130-1130, 1609-1609, 1682-1682

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/manager.test.ts` at line 1086, Replace
hardcoded long setTimeout waits in the tests with deterministic timing: either
shorten the delay to a small value (50–100ms) where rate-limiting behavior is
not under test, or switch the test suite to use
vi.useFakeTimers()/vi.runAllTimersAsync() and vi.useRealTimers() in
beforeEach/afterEach so you can emit events (e.g., via connector.emit("message",
event)) and then advance timers deterministically instead of awaiting new
Promise(resolve => setTimeout(resolve, 2000+)). Update the occurrences of the
Promise+setTimeout pattern in the manager.test.ts tests (the places that emit
connector events and then wait for rate limiting) to use the fake timer approach
or reduced delays.

packages/cli/src/commands/__tests__/agent.test.ts (1)

315-316: Optional: deduplicate repeated self_scheduling fixture payload.

The same object is repeated several times; extracting one constant will make future updates less error-prone.

Small DRY refactor

+const DEFAULT_SELF_SCHEDULING = { enabled: true, max_schedules: 10, min_interval: "5m" };
...
-      agentConfig: { name: agentName, permission_mode: "default", runtime: "sdk", self_scheduling: { enabled: true, max_schedules: 10, min_interval: "5m" } },
+      agentConfig: { name: agentName, permission_mode: "default", runtime: "sdk", self_scheduling: DEFAULT_SELF_SCHEDULING },

Also applies to: 371-372, 569-570, 632-633

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/__tests__/agent.test.ts` around lines 315 - 316,
Extract the repeated self_scheduling object into a single constant (e.g.,
SELF_SCHEDULING_FIXTURE) and reuse it inside the various agentConfig fixtures
instead of inlining the object; update the test cases that construct agentConfig
(the entries containing permission_mode, runtime, and self_scheduling) to
reference SELF_SCHEDULING_FIXTURE, and replace the repeated literals at the
other occurrences noted in the comment so edits to scheduling params only need
to be made in one place.

🤖 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/schema.ts`:
- Line 629: Update the stale inline docs/comments for the Discord output
configuration so the documented default for the result_summary option matches
the actual schema default (false). Locate the occurrences of "result_summary" in
packages/core/src/config/schema.ts (the comment block around the Discord output
example and the related comments at the other two occurrences) and change the
example/comment that currently says "result_summary: true" to "result_summary:
false" (or otherwise note the default is false) so documentation and the schema
(result_summary default false) are consistent.
- Line 991: The self_scheduling schema currently defaults enabled to true which
turns the feature on for all agents; update the schema property
(self_scheduling.enabled) to default to false instead of true to make it opt-in,
and apply the same change to the other occurrence referenced in the file (the
second self_scheduling.enabled block around the 1043-1045 area) so both schema
instances are consistent. Ensure you only change the default value for the
enabled key and leave surrounding validation (z.boolean()) intact.
- Around line 736-742: The current Zod schema for download_dir (the download_dir
field in packages/core/src/config/schema.ts) only rejects POSIX
absolute/traversal patterns and still permits Windows absolute paths (e.g.,
C:\...) and backslash traversal (..\). Update the refine predicate to also
reject Windows drive-letter absolute paths and UNC/leading-backslash paths by
checking for patterns like /^[A-Za-z]:\\/ and /^\\\\/ and also reject
backslash-based parent segments (e.g., contains "..\\" or startsWith("\\"));
alternatively use Node's path utilities (path.isAbsolute and path.normalize)
inside the refine to ensure the value is a relative path without any ".."
segments after normalization. Ensure the error message remains descriptive and
keep the validation attached to the same download_dir Zod chain.

In `@packages/discord/src/__tests__/discord-connector.test.ts`:
- Line 128: The shared test fixture currently hardcodes result_summary: true;
change that default to result_summary: false in the shared config so tests
exercise the reverted default, and update any tests that need a true behavior to
set result_summary: true explicitly in their individual fixtures or test setup;
look for the shared fixture object that contains the "result_summary" property
and flip its value and adjust dependent tests accordingly.

---

Nitpick comments:
In `@packages/cli/src/commands/__tests__/agent.test.ts`:
- Around line 315-316: Extract the repeated self_scheduling object into a single
constant (e.g., SELF_SCHEDULING_FIXTURE) and reuse it inside the various
agentConfig fixtures instead of inlining the object; update the test cases that
construct agentConfig (the entries containing permission_mode, runtime, and
self_scheduling) to reference SELF_SCHEDULING_FIXTURE, and replace the repeated
literals at the other occurrences noted in the comment so edits to scheduling
params only need to be made in one place.

In `@packages/discord/src/__tests__/manager.test.ts`:
- Around line 580-582: The test mock for the reply function currently prints to
stdout; replace the inline console.log in the mock reply implementation with a
no-op or a jest mock so tests don't clutter output—e.g., change the reply mock
(reply: async (content) => { console.log("Reply:", content); }) to a silent
implementation or use jest.fn()/sinon stub to record calls (reply:
jest.fn().mockResolvedValue(undefined)) so assertions can still inspect calls
without printing.
- Line 1086: Replace hardcoded long setTimeout waits in the tests with
deterministic timing: either shorten the delay to a small value (50–100ms) where
rate-limiting behavior is not under test, or switch the test suite to use
vi.useFakeTimers()/vi.runAllTimersAsync() and vi.useRealTimers() in
beforeEach/afterEach so you can emit events (e.g., via connector.emit("message",
event)) and then advance timers deterministically instead of awaiting new
Promise(resolve => setTimeout(resolve, 2000+)). Update the occurrences of the
Promise+setTimeout pattern in the manager.test.ts tests (the places that emit
connector events and then wait for rate limiting) to use the fake timer approach
or reduced delays.

In `@packages/slack/src/__tests__/manager.test.ts`:
- Line 73: Extract the repeated self_scheduling literal into a single shared
constant (e.g., const SELF_SCHEDULING = { enabled: true, max_schedules: 10,
min_interval: "5m" }) and replace the duplicate inline objects in the test file
with references to that constant; update occurrences used in the tests that
currently define self_scheduling inline (the fixtures around the manager tests)
to use SELF_SCHEDULING so future changes only require editing one place.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4011defe-725d-4118-912e-4aeac354e398

📥 Commits

Reviewing files that changed from the base of the PR and between 069b82c and 700c57f.

📒 Files selected for processing (11)

• packages/cli/src/commands/__tests__/agent.test.ts
• packages/core/src/config/__tests__/merge.test.ts
• packages/core/src/config/schema.ts
• packages/core/src/runner/__tests__/job-executor.test.ts
• packages/core/src/runner/__tests__/sdk-adapter.test.ts
• packages/core/src/scheduler/__tests__/scheduler.test.ts
• packages/core/src/work-sources/__tests__/manager.test.ts
• packages/discord/src/__tests__/discord-connector.test.ts
• packages/discord/src/__tests__/manager.test.ts
• packages/slack/src/__tests__/manager.test.ts
• packages/web/src/server/chat/web-chat-manager.ts

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Fix stale default value in Discord output docs/comments.

The comments say result_summary defaults to true, but schema default is false. This will confuse operators reading config examples.

Suggested doc fix

- *   result_summary: true
+ *   result_summary: false
...
- /** Show a summary embed when the agent finishes a turn (cost, tokens, turns) (default: true) */
+ /** Show a summary embed when the agent finishes a turn (cost, tokens, turns) (default: false) */

Also applies to: 636-637

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/schema.ts` at line 629, Update the stale inline
docs/comments for the Discord output configuration so the documented default for
the result_summary option matches the actual schema default (false). Locate the
occurrences of "result_summary" in packages/core/src/config/schema.ts (the
comment block around the Discord output example and the related comments at the
other two occurrences) and change the example/comment that currently says
"result_summary: true" to "result_summary: false" (or otherwise note the default
is false) so documentation and the schema (result_summary default false) are
consistent.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Harden download_dir validation against Windows absolute/traversal paths.

Current refine allows values like C:\temp or ..\outside on Windows-style inputs. That weakens path-boundary guarantees for attachment writes.

Suggested validation hardening

   download_dir: z
     .string()
     .optional()
     .default(".discord-attachments")
-    .refine((v) => !v.includes("..") && !v.startsWith("/"), {
+    .refine((v) => {
+      // Reject absolute paths (POSIX, UNC, Windows drive letters)
+      if (/^(\/|\\\\|[a-zA-Z]:[\\/])/.test(v)) return false;
+      // Reject parent traversal in either separator style
+      if (/(^|[\\/])\.\.([\\/]|$)/.test(v)) return false;
+      return true;
+    }, {
       message: "download_dir must be a relative path without '..' segments",
     }),

As per coding guidelines: "Use Zod schemas for validation of all external input including config files, CLI arguments, and API payloads".

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/schema.ts` around lines 736 - 742, The current Zod
schema for download_dir (the download_dir field in
packages/core/src/config/schema.ts) only rejects POSIX absolute/traversal
patterns and still permits Windows absolute paths (e.g., C:\...) and backslash
traversal (..\). Update the refine predicate to also reject Windows drive-letter
absolute paths and UNC/leading-backslash paths by checking for patterns like
/^[A-Za-z]:\\/ and /^\\\\/ and also reject backslash-based parent segments
(e.g., contains "..\\" or startsWith("\\")); alternatively use Node's path
utilities (path.isAbsolute and path.normalize) inside the refine to ensure the
value is a relative path without any ".." segments after normalization. Ensure
the error message remains descriptive and keep the validation attached to the
same download_dir Zod chain.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Defaulting self_scheduling.enabled to true is an aggressive global behavior change.

With self_scheduling auto-defaulted in agent config, this enables capability by default for all agents unless explicitly disabled. Consider making it opt-in (enabled: false) to preserve least-privilege behavior.

Safer default

 export const SelfSchedulingSchema = z.object({
   /** Whether self-scheduling is enabled for this agent */
-  enabled: z.boolean().default(true),
+  enabled: z.boolean().default(false),

Also applies to: 1043-1045

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/config/schema.ts` at line 991, The self_scheduling schema
currently defaults enabled to true which turns the feature on for all agents;
update the schema property (self_scheduling.enabled) to default to false instead
of true to make it opt-in, and apply the same change to the other occurrence
referenced in the file (the second self_scheduling.enabled block around the
1043-1045 area) so both schema instances are consistent. Ensure you only change
the default value for the enabled key and leave surrounding validation
(z.boolean()) intact.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Align shared fixture default with result_summary: false.

At Line 128, the shared config hardcodes result_summary: true. Since the default was reverted to false, this baseline can mask fallback/default regressions in tests that rely on fixture defaults.

Suggested tweak

-      result_summary: true,
+      result_summary: false,

📝 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

	result_summary: true,
	result_summary: false,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@packages/discord/src/__tests__/discord-connector.test.ts` at line 128, The
shared test fixture currently hardcodes result_summary: true; change that
default to result_summary: false in the shared config so tests exercise the
reverted default, and update any tests that need a true behavior to set
result_summary: true explicitly in their individual fixtures or test setup; look
for the shared fixture object that contains the "result_summary" property and
flip its value and adjust dependent tests accordingly.