fix: include webhook_reply and cron_cursor_job_id in version hash#4599
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes workflow version hash generation so that trigger changes to webhook_reply and cron_cursor_job_id are reflected in the hash, ensuring CLI deploy and sandbox merge correctly detect those changes (per #4596).
Changes:
- Extend
WorkflowVersions.generate_hash/1trigger hashing to include:webhook_replyand:cron_cursor_job_id. - Add regression tests asserting the hash changes when either of those trigger fields changes.
- Document the fix in the changelog under Unreleased / Fixed.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
lib/lightning/workflow_versions.ex |
Adds webhook_reply and cron_cursor_job_id to the trigger field list used for deterministic hashing. |
test/lightning/workflow_versions_test.exs |
Adds tests verifying hash changes when webhook_reply or cron_cursor_job_id changes. |
CHANGELOG.md |
Notes the hashing fix in the Unreleased “Fixed” section with issue link. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Hey @officialasishkumar, thanks so much for this contribution, really appreciate you picking this up!
The change itself makes sense. One thing to consider though: the CLI computes version hashes using the same algorithm, and its trigger keys need to stay in sync with the server.
In kit/packages/project/src/util/version.ts (line 57), the CLI currently has:
const triggerKeys = ['type', 'cron_expression', 'enabled'].sort();This needs to be updated to:
const triggerKeys = ['type', 'cron_expression', 'enabled', 'webhook_reply', 'cron_cursor_job_id'].sort();Without this, the CLI and server will produce different hashes for the same workflow, and deploy will think workflows changed when they haven't.
Would you mind opening a companion PR in the kit repo with that update?
|
There's a small merge conflict in CHANGELOG.md, just needs a rebase onto main and moving the entry under the current |
fac2f8a to
de093f5
Compare
|
@elias-ba Rebased onto current |
|
Opened the companion change in OpenFn/kit as #1362. This branch is also current on |
|
Hi @officialasishkumar, your AI disclaimer says you haven't used AI to produce this PR (including research, comments and the PR description). Is that true? |
The version hash calculation in WorkflowVersions.generate_hash/1 did not include the webhook_reply or cron_cursor_job_id trigger fields. This meant that changes to these fields were not detected by CLI deploy or sandbox merge, since the hash remained unchanged. Add both fields to the trigger_keys list used in hash generation, and add tests verifying that changing either field produces a different hash. Signed-off-by: Asish Kumar <officialasishkumar@gmail.com>
de093f5 to
746f4a7
Compare
|
@josephjclark Yes, used only for research. |
|
@officialasishkumar Thanks for being upfront about it, we really appreciate that. To be clear, we actively encourage contributors to use AI tools. We all rely on them to deliver faster and better work, and there's absolutely no stigma attached. That said, transparency is important to us. Whether AI was used for research, fact-checking, writing docs, drafting code, or anything else, it all counts as AI usage. The "I have not used AI" checkbox should only be ticked if you genuinely didn't use any form of AI assistance at all during your work on this PR. Would you mind updating the checkboxes to reflect your actual usage? Either "I have used Claude Code" or "I have used another model" would be appropriate here. Once you've done that, please re-request a review and I'll approve this right away. |
|
Updated @elias-ba |
Description
This PR fixes the version hash calculation to include
webhook_replyandcron_cursor_job_idtrigger fields. Previously,WorkflowVersions.generate_hash/1only hashed:type,:cron_expression, and:enabledfrom triggers, meaning changes to webhook reply mode or cron cursor job were invisible to CLI deploy and sandbox merge.Closes #4596
Additional notes for the reviewer
trigger_keyslist ingenerate_hash/1. No structural changes to the hash algorithm.AI Usage
Used only for research.
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)