fix: register workflow triggers on apply#162
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughWorkflow trigger reconciliation is moved from an unused server-side ChangesTrigger registration fix
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant WorkflowStore as workflow.Save
participant DB as Database (Tx)
participant EmailPoller as EmailTriggerPoller
Client->>WorkflowStore: Save(workflow YAML)
WorkflowStore->>DB: BEGIN transaction
WorkflowStore->>DB: INSERT workflow_definitions
WorkflowStore->>DB: syncTriggersTx (DELETE + INSERT workflow_triggers)
WorkflowStore->>DB: COMMIT
DB-->>WorkflowStore: success
loop every emailReloadInterval
EmailPoller->>DB: Reload triggers
DB-->>EmailPoller: updated trigger set
end
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d058c26979
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Register the workflow's declared triggers so the cron/webhook/email | ||
| // schedulers actually fire it. Done in the same transaction as the | ||
| // definition insert so version and triggers commit atomically. | ||
| if err := syncTriggersTx(ctx, tx, teamID, name, newVersion, result.Workflow.Triggers); err != nil { |
There was a problem hiding this comment.
Because Save still returns 0, nil when latestHash == hash before reaching this new reconciliation call, re-applying an already-saved workflow after upgrading to this fix will not backfill workflow_triggers. That is the common recovery path for workflows created before this change, where workflow_definitions already has the matching hash but the trigger table is empty, so cron/webhook/email triggers remain inert until the user makes a byte-level YAML change.
Useful? React with 👍 / 👎.
| `INSERT INTO workflow_triggers | ||
| (workflow_name, workflow_version, type, schedule, path, secret, team_id, mailbox, folder, filter, poll_interval) | ||
| VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)`, |
There was a problem hiding this comment.
This insert now makes applies fail in multi-tenant deployments when another team already registered the same webhook path, or the same workflow name and cron schedule, because the existing unique indexes on workflow_triggers are not team-scoped (path for webhooks and (workflow_name, schedule) for cron). Since the rest of the trigger read paths carry/use team_id, two teams should be able to apply the same workflow or webhook path under their own auth context; the new registration path needs a migration to include team_id in those unique constraints before relying on this insert.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/engine/internal/workflow/store.go (1)
32-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winBackfill triggers on the unchanged-content Save path.
latestHash == hashreturns beforesyncTriggersTx, so workflows applied before this fix can still have zeroworkflow_triggersrows after a no-opmantle apply/GitOps sync. Reconcile the latest version’s triggers on this path too, preserving disabled state if applicable, and add a regression test that seeds a definition without trigger rows then applies identical content.Also applies to: 61-65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/internal/workflow/store.go` around lines 32 - 34, The unchanged-content branch in `Store.Save` returns before `syncTriggersTx`, which skips trigger reconciliation for existing workflows with stale or missing rows. Update the `Save`/`syncTriggersTx` flow so the no-op hash path still reconciles the latest version’s triggers, while preserving disabled state when present, and add a regression test that seeds a definition without `workflow_triggers` rows and then applies identical content to verify the rows are backfilled.
🧹 Nitpick comments (1)
packages/engine/internal/workflow/triggers_test.go (1)
47-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing field-level assertions for the webhook trigger.
The test verifies cron (
schedule) and email (mailbox,folder,filter,poll_interval) field persistence, but the webhook trigger'spath(/incoming) andsecret(shh) values declared in the YAML are never asserted — only the total row count (3) is checked. Since this is the regression test for the bug where trigger data silently failed to persist, verifying the webhook column mapping would close a real coverage gap analogous to the email trigger assertions already present.✅ Suggested addition
+ // The webhook trigger row must carry its path and secret. + var path, secret string + if err := database.QueryRowContext(ctx, + `SELECT COALESCE(path,''), COALESCE(secret,'') FROM workflow_triggers + WHERE workflow_name = 'triggered-wf' AND type = 'webhook'`, + ).Scan(&path, &secret); err != nil { + t.Fatalf("querying webhook trigger: %v", err) + } + if path != "/incoming" || secret != "shh" { + t.Errorf("webhook fields = (%q,%q), want (/incoming,shh)", path, secret) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/internal/workflow/triggers_test.go` around lines 47 - 100, The regression test in TestSave_RegistersTriggers covers cron and email trigger persistence but misses webhook field-level validation. Add assertions for the webhook row in workflow_triggers, using the existing Save, countTriggers, and database.QueryRowContext checks to verify the webhook-specific path and secret values from triggeredWorkflowYAML persist correctly alongside the row count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/engine/internal/workflow/store.go`:
- Around line 32-34: The unchanged-content branch in `Store.Save` returns before
`syncTriggersTx`, which skips trigger reconciliation for existing workflows with
stale or missing rows. Update the `Save`/`syncTriggersTx` flow so the no-op hash
path still reconciles the latest version’s triggers, while preserving disabled
state when present, and add a regression test that seeds a definition without
`workflow_triggers` rows and then applies identical content to verify the rows
are backfilled.
---
Nitpick comments:
In `@packages/engine/internal/workflow/triggers_test.go`:
- Around line 47-100: The regression test in TestSave_RegistersTriggers covers
cron and email trigger persistence but misses webhook field-level validation.
Add assertions for the webhook row in workflow_triggers, using the existing
Save, countTriggers, and database.QueryRowContext checks to verify the
webhook-specific path and secret values from triggeredWorkflowYAML persist
correctly alongside the row count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 47c5f431-ff11-466e-a588-e6d1adc255f6
📒 Files selected for processing (6)
.changeset/fix-trigger-registration.mdpackages/engine/internal/server/email_trigger.gopackages/engine/internal/server/trigger.gopackages/engine/internal/workflow/store.gopackages/engine/internal/workflow/triggers.gopackages/engine/internal/workflow/triggers_test.go
|
Thanks for the reviews. Status on the feedback: ✅ Backfill on unchanged re-apply (Codex P1 / CodeRabbit Major) — fixed in ✅ Webhook field assertions (CodeRabbit nitpick) — added ⏭️ Team-scoped trigger uniqueness (Codex P2) — valid, but deferring to a separate change. The unique indexes ( CI
Generated by Claude Code |
Cron, webhook, and email triggers declared in a workflow's YAML were parsed and validated but never written to workflow_triggers, because the only function that populated that table (server.SyncTriggers) had no callers. The cron scheduler, webhook handler, and email poller all read from that table, so declared triggers never fired. - workflow.Save now reconciles workflow_triggers in the same transaction as the definition insert, so both `mantle apply` and GitOps sync register a workflow's triggers atomically with its new version. - workflow.Disable/Reenable toggle the triggers' enabled flag so a pruned/disabled workflow stops firing (schedulers key off workflow_triggers.enabled, not the definition's disabled_at) and a re-enabled unchanged workflow gets its triggers back. - The email poller periodically reconciles against the DB so email triggers registered by a later apply (a separate process) or a GitOps sync activate without a server restart. Cron and webhook triggers are already read live per tick/request. - Removed the dead server.SyncTriggers/TriggerInput; trigger-write logic now lives in the workflow package (avoids an import cycle). Adds integration tests covering registration, version replacement, the no-trigger case, and the disable/reenable toggle. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKgCRfMvFcZAkoVbr8k79
Addresses PR review feedback on the trigger-registration fix. - workflow.Save reconciles triggers on the unchanged-content path too, so workflows applied before this fix (definition present, workflow_triggers empty) register their triggers on a plain re-apply instead of requiring a byte-level YAML change. Reconciliation only runs when the stored rows have actually drifted from the latest version's declared triggers, so steady-state GitOps re-applies don't churn rows (which matters for the email poller, whose IMAP connections key off trigger IDs). - Backfilled rows inherit the workflow's enabled/disabled state, so a disabled workflow doesn't start firing on re-apply. - Tests: assert webhook path/secret persistence, add backfill regression (including no-churn on repeat apply) and disabled-state preservation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WnKgCRfMvFcZAkoVbr8k79
michaelmcnees
force-pushed
the
claude/office-assistant-mantle-feasibility-n8f4bk
branch
from
8b3339f to
b5da9b0
Compare
2 participants
Summary
Cron, webhook, and email triggers declared in a workflow's YAML were parsed and validated but never fired. The only function that populated the
workflow_triggerstable (server.SyncTriggers) had zero callers, so the table stayed empty — and the cron scheduler, webhook handler, and email poller all read from it. This makes trigger registration actually happen on apply.Changes
workflow.Savereconcilesworkflow_triggersin the same transaction as the definition insert, so bothmantle applyand GitOps sync register a workflow's triggers atomically with each new version.workflow.Disable/Reenabletoggle the triggers'enabledflag. The schedulers key offworkflow_triggers.enabled(not the definition'sdisabled_at), so a pruned/disabled workflow would otherwise keep firing.Reenablealso restores triggers for the case where a pruned workflow reappears with byte-identical content (whereSaveis a no-op).mantle apply(a separate process) or a GitOps sync activate without a server restart. Cron and webhook triggers are already read live per tick/request, so they need no equivalent.server.SyncTriggers/TriggerInput; the trigger-write logic now lives in theworkflowpackage (avoids an import cycle, sinceserveralready importsworkflow).Testing
make testpasses — unit tests (cron matching, email poller start/stop/reload) pass locally; the new integration tests are testcontainer-based and require Docker, which was unavailable in the dev sandbox (the pre-existingTestSave_*tests hit the same limitation). They should run on CI.make lint—go build ./...andgo vetpass;golangci-lintnot run locally.internal/workflow/triggers_test.gocovers registration on apply, version replacement (no duplicate accumulation), the no-trigger case, and the disable/reenable enabled-toggle.Related Issues
Closes #160
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes