Skip to content

fix(core): skip Discord hooks for Discord-triggered jobs#214

Open
oheckmann74 wants to merge 3 commits into
edspencer:mainfrom
oheckmann74:fix/discord-duplicate-hooks-upstream
Open

fix(core): skip Discord hooks for Discord-triggered jobs#214
oheckmann74 wants to merge 3 commits into
edspencer:mainfrom
oheckmann74:fix/discord-duplicate-hooks-upstream

Conversation

@oheckmann74

@oheckmann74 oheckmann74 commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Summary

  • When a job is triggered from Discord, the Discord manager already streams output to the channel in real-time via onMessage. After the job completes, executeHooks also fires any configured discord-type hooks (after_run and on_error), resulting in duplicate messages in the Discord channel.
  • This fix filters out Discord-type hooks when the job's triggerType is "discord", preventing the duplication while leaving all other hook types (shell, webhook, slack) unaffected.
  • Adds triggerType to HookContext so hook execution can be trigger-aware.

Changes

  • packages/core/src/fleet-manager/job-control.ts: Filter Discord hooks from after_run and on_error arrays when context.triggerType === "discord". Plumb triggerType from jobMetadata.trigger_type into buildHookContext().
  • packages/core/src/hooks/types.ts: Add optional triggerType field to HookContext interface with documentation.

Test plan

  • Configure an agent with a Discord after_run hook and trigger a job from Discord — verify only one response appears (not duplicated)
  • Trigger the same agent via CLI/schedule — verify the Discord hook still fires normally
  • Verify non-Discord hooks (shell, webhook) still execute for Discord-triggered jobs

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Prevented duplicate notifications when jobs are triggered from Discord by skipping redundant Discord-specific hooks during execution (including success and failure cases).
  • Improvements

    • Job execution context now recognizes trigger sources (Discord, manual, scheduled, Slack) so notifications and hooks are handled more accurately and consistently.

…uplicate messages

When a job is triggered from Discord, the Discord manager already streams
all output to the channel in real-time via the onMessage callback. The
hooks system was also firing Discord after_run/on_error hooks, causing
duplicate messages in the channel.

This fix:
- Adds triggerType to HookContext so hooks know how a job was triggered
- Populates triggerType from job metadata's trigger_type field
- Filters out Discord-type hooks when triggerType is "discord"

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@oheckmann74 oheckmann74 requested a review from edspencer as a code owner March 19, 2026 04:32
@coderabbitai

coderabbitai Bot commented Mar 19, 2026

Copy link
Copy Markdown
Contributor

Warning

Rate limit exceeded

@oheckmann74 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 25 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e1b4e2dc-08a5-47bc-87bb-3f5b1686bb58

📥 Commits

Reviewing files that changed from the base of the PR and between ac91401 and 90237e1.

📒 Files selected for processing (1)
  • packages/core/src/hooks/types.ts
📝 Walkthrough

Walkthrough

Job hook execution now omits Discord-type hooks when the job's trigger is Discord. HookContext gained an optional triggerType property so the controller can detect Discord-triggered jobs and filter after_run and on_error hooks before executing them.

Changes

Cohort / File(s) Summary
Hook filtering & context
packages/core/src/fleet-manager/job-control.ts, packages/core/src/hooks/types.ts
Added triggerType?: TriggerType to HookContext. In job-control, filter out hooks with type === "discord" from after_run and on_error when context.triggerType === "discord", log when filters reduce hooks, and pass filtered hooks to the hook executor.

Sequence Diagram

sequenceDiagram
    participant Trigger as Job Trigger
    participant Controller as Job Controller
    participant Executor as Hook Executor

    Trigger->>Controller: start job (includes triggerType)
    Controller->>Controller: buildHookContext (includes triggerType)
    Controller->>Controller: if triggerType == "discord" then remove discord hooks from after_run/on_error
    Controller->>Controller: log if hooks were filtered
    Controller->>Executor: executeHooks(filtered hooks, hookContext)
    Executor-->>Controller: execution result
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped in quick to trim the thread,
Discord echoes softly fled,
Hooks now sing just once, not twice,
Neat and tidy—what a slice! ✨

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: filtering Discord hooks when jobs are Discord-triggered to prevent duplicate messages.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
📝 Coding Plan
  • Generate coding plan for human review comments

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/core/src/hooks/types.ts (1)

152-159: Consider using the TriggerType enum for stronger type safety.

The triggerType field is typed as string, but the TriggerTypeSchema in state/schemas/job-metadata.ts defines a specific set of valid values ("manual", "schedule", "webhook", "chat", "discord", "slack", "web", "fork"). Using the inferred TriggerType type would provide better type safety and IDE autocomplete.

♻️ Suggested improvement
+import type { TriggerType } from "../state/schemas/job-metadata.js";
+
 /**
  * How the job was triggered (e.g. "manual", "schedule", "discord", "slack")
  *
  * Used to prevent duplicate notifications - for example, Discord hooks are
  * skipped when triggerType is "discord" because the Discord manager already
  * streams output to the channel in real-time.
  */
-triggerType?: string;
+triggerType?: TriggerType;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/hooks/types.ts` around lines 152 - 159, The triggerType
property is currently typed as string; change it to use the project's
TriggerType type/enum for stronger typing and autocomplete by importing the
TriggerType (or the exported type inferred from TriggerTypeSchema) from the
job-metadata schema module and updating the declaration triggerType?: string; to
triggerType?: TriggerType; (and adjust any related imports/usages in functions
that read or set triggerType to match the enum values). Ensure you reference the
TriggerType (or exported type name from state/schemas/job-metadata.ts) and
update any consumers of the Hooks types to accept the narrowed type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/core/src/hooks/types.ts`:
- Around line 152-159: The triggerType property is currently typed as string;
change it to use the project's TriggerType type/enum for stronger typing and
autocomplete by importing the TriggerType (or the exported type inferred from
TriggerTypeSchema) from the job-metadata schema module and updating the
declaration triggerType?: string; to triggerType?: TriggerType; (and adjust any
related imports/usages in functions that read or set triggerType to match the
enum values). Ensure you reference the TriggerType (or exported type name from
state/schemas/job-metadata.ts) and update any consumers of the Hooks types to
accept the narrowed type.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: fded9310-6a52-4c60-a4fb-32b570681a43

📥 Commits

Reviewing files that changed from the base of the PR and between 3662d18 and a94da4f.

📒 Files selected for processing (2)
  • packages/core/src/fleet-manager/job-control.ts
  • packages/core/src/hooks/types.ts

Addresses CodeRabbit review feedback — import and use the TriggerType
type from job-metadata schema for stronger type safety and IDE autocomplete.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/core/src/hooks/types.ts (1)

154-162: Consider documenting all trigger variants (or referencing the schema directly).

The comment currently shows only a subset of TriggerType values; this can drift from TriggerTypeSchema and mislead future edits. Prefer either listing all enum values or explicitly saying this is a non-exhaustive subset tied to job-metadata.ts.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/core/src/hooks/types.ts` around lines 154 - 162, Update the doc
comment for the TriggerType field to avoid misleading readers by either
enumerating every variant from TriggerType (or TriggerTypeSchema) or explicitly
marking the list as non-exhaustive and pointing to the canonical source;
specifically update the comment above triggerType?: TriggerType in types.ts to
reference TriggerTypeSchema or job-metadata.ts (or list all enum values from
TriggerType) so future changes to TriggerType/TriggerTypeSchema remain
authoritative.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/core/src/hooks/types.ts`:
- Around line 154-162: Update the doc comment for the TriggerType field to avoid
misleading readers by either enumerating every variant from TriggerType (or
TriggerTypeSchema) or explicitly marking the list as non-exhaustive and pointing
to the canonical source; specifically update the comment above triggerType?:
TriggerType in types.ts to reference TriggerTypeSchema or job-metadata.ts (or
list all enum values from TriggerType) so future changes to
TriggerType/TriggerTypeSchema remain authoritative.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: be7a951c-40d1-42e8-a5bc-e512f0da7cb8

📥 Commits

Reviewing files that changed from the base of the PR and between a94da4f and ac91401.

📒 Files selected for processing (1)
  • packages/core/src/hooks/types.ts

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant