feat(triggers): unify PM trigger naming to pm:status-changed#605
feat(triggers): unify PM trigger naming to pm:status-changed#605zbigniewsobiecki merged 4 commits intodevfrom
Conversation
Consolidates the provider-specific `pm:card-moved` (Trello) and `pm:issue-transitioned` (Jira) events into a single `pm:status-changed` trigger that works across all PM integrations at the category level. Key changes: - TRIGGER_REGISTRY: replace two PM entries with unified pm:status-changed - Agent YAMLs: merge two triggers into single pm:status-changed per agent - New handlers: TrelloStatusChangedTrigger, JiraStatusChangedTrigger - New Trello types: moved TrelloWebhookPayload to src/triggers/trello/types.ts - Config schema: add statusChanged key (replaces cardMovedTo*/issueTransitioned) - Backward compat: legacy config keys still parsed and resolved - Migration tool: maps old event names to pm:status-changed - All tests updated/rewritten for new naming Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
✨ On it — checking the feature/unify-pm-status-changed-trigger branch Progress: [░░░░░░░░░░] 3% (iteration 2/70) 🔍 Code Review Update (1 min) I've started reviewing the pull request by fetching the diff. I'm now examining the changes to identify any potential issues and ensure the completeness of the proposed solution. My next step is to focus on key files not initially loaded to verify the changes. Last updated: iteration 2 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Well-structured unification of PM trigger naming. The implementation is correct and thorough across all 31 files. One documentation issue to address.
Should Fix
- CLAUDE.md (lines 220, 242, 245, 283-284, 289, 292) — The CLAUDE.md documentation still references
pm:card-movedandpm:issue-transitionedthroughout the "Agent Trigger Configuration" section. Since CLAUDE.md serves as the primary developer guide (and is injected into agent context), these stale references will confuse both developers and agents. Should be updated topm:status-changedto match the new naming.
Observations (non-blocking)
-
Trello per-list granularity: The old config allowed disabling triggers per-list (
cardMovedToSplitting: falsewhile keepingcardMovedToTodo: true). The new TrellostatusChangedis a single boolean — all three lists are toggled together. The Jira side supports per-agent granularity viaStatusChangedSchema. This seems intentional (the JIRA trigger fires for any status change, so per-agent makes sense there, while Trello triggers are already list-specific), but worth noting since it's a capability regression for Trello users who had per-list toggles. -
Old files retained:
src/triggers/trello/card-moved.tsandsrc/triggers/jira/issue-transitioned.tsstill exist on disk but are no longer imported from anysrc/code. The barrel exports have been updated to point to the new files. Consider removing them in a follow-up to avoid confusion, or add a deprecation comment at the top of each file. -
Implementation quality: The factory pattern, backward compat via
resolveStatusChangedEnabledfallback, re-exports for type compatibility, and test coverage (including per-agent toggle tests, case-insensitive matching, and edge cases) are all solid.
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Well-structured unification of PM trigger naming. The implementation is correct and thorough across all 31 files. One documentation gap to address as a follow-up.
Should Fix (follow-up)
- CLAUDE.md (lines 220, 242, 245, 283-284, 289, 292) — The CLAUDE.md documentation still references
pm:card-movedandpm:issue-transitionedthroughout the "Agent Trigger Configuration" section. Since CLAUDE.md serves as the primary developer guide (and is injected into agent context), these stale references will confuse both developers and agents. Should be updated topm:status-changedto match the new naming.
Observations (non-blocking)
-
Trello per-list granularity: The old config allowed disabling triggers per-list (
cardMovedToSplitting: falsewhile keepingcardMovedToTodo: true). The new TrellostatusChangedis a single boolean — all three lists are toggled together. The Jira side supports per-agent granularity viaStatusChangedSchema. This seems intentional (Trello triggers are already list-specific by nature), but worth noting since it's a capability regression for Trello users who had per-list toggles. -
Old files retained:
src/triggers/trello/card-moved.tsandsrc/triggers/jira/issue-transitioned.tsstill exist on disk but are no longer imported from anysrc/code. Consider removing them in a follow-up to avoid confusion. -
Implementation quality: Factory pattern, backward compat via
resolveStatusChangedEnabledfallback, re-exports for type compatibility, and test coverage (per-agent toggle tests, case-insensitive matching, edge cases) are all solid.
|
@aaight address code review concerns |
Request AddressedWhat was asked@zbigniewsobiecki asked to address code review concerns from @nhopeatall's review. Changes Made
Verification
|
Replace stale pm:card-moved and pm:issue-transitioned references with the unified pm:status-changed event name throughout the Agent Trigger Configuration section. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the CLAUDE.md documentation concern from @nhopeatall's review. Updated all stale
Commit: ae23250 |
|
✅ Review complete for feature/unify-pm-status-changed-trigger Verdict: COMMENT — one housekeeping item, otherwise solid work. Summary: Well-structured unification of Should Fix:
Design Note: Trello |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured unification of PM trigger naming. The consolidation of pm:card-moved and pm:issue-transitioned into a single pm:status-changed event is a good simplification. Backward compatibility is handled carefully with config key fallbacks, deprecated aliases, type re-exports, and a migration tool update. CI is green and test coverage is comprehensive. One housekeeping item to address.
Should Fix
- Dead code left behind: The old trigger handlers
src/triggers/trello/card-moved.tsandsrc/triggers/jira/issue-transitioned.tsstill exist on disk but are no longer imported by any production code (register files, barrel exports, etc. all reference the newstatus-changed.tsfiles). Their corresponding test files (tests/unit/triggers/card-moved.test.ts,tests/unit/triggers/jira-issue-transitioned.test.ts) are also orphaned. These should be deleted in this PR to avoid confusion — someone browsing the codebase would see both the old and new triggers side by side with no clear indication that the old ones are dead.
Minor (not in diff, FYI only)
src/triggers/jira/label-added.tslines 6 and 9 still referenceissue-transitionedandJiraIssueTransitionedTriggerin comments. Not blocking since the file isn't part of this PR, but worth a follow-up cleanup.
Design Note
The Trello statusChanged config key is a single boolean replacing three independent per-list keys (cardMovedToSplitting, cardMovedToPlanning, cardMovedToTodo). This is a granularity reduction in the legacy JSONB path — but the new agent_trigger_configs table provides per-agent per-event control, and the migration tool correctly maps the old per-list keys to per-agent rows in the new table. So the net effect is fine for migrated projects. Just noting this is an intentional design tradeoff.
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured unification of PM trigger naming. The consolidation of pm:card-moved and pm:issue-transitioned into a single pm:status-changed event is a good simplification. Backward compatibility is handled carefully with config key fallbacks, deprecated aliases, type re-exports, and a migration tool update. CI is green and test coverage is comprehensive. One housekeeping item to address.
Should Fix
- Dead code left behind: The old trigger handlers
src/triggers/trello/card-moved.tsandsrc/triggers/jira/issue-transitioned.tsstill exist on disk but are no longer imported by any production code (register files, barrel exports, etc. all reference the newstatus-changed.tsfiles). Their corresponding test files (tests/unit/triggers/card-moved.test.ts,tests/unit/triggers/jira-issue-transitioned.test.ts) are also orphaned. These should be deleted in this PR to avoid confusion — someone browsing the codebase would see both the old and new triggers side by side with no clear indication that the old ones are dead.
Minor (not in diff, FYI only)
src/triggers/jira/label-added.tslines 6 and 9 still referenceissue-transitionedandJiraIssueTransitionedTriggerin comments. Not blocking since the file isn't part of this PR, but worth a follow-up cleanup.
Design Note
The Trello statusChanged config key is a single boolean replacing three independent per-list keys (cardMovedToSplitting, cardMovedToPlanning, cardMovedToTodo). This is a granularity reduction in the legacy JSONB path — but the new agent_trigger_configs table provides per-agent per-event control, and the migration tool correctly maps the old per-list keys to per-agent rows in the new table. So the net effect is fine for migrated projects. Just noting this is an intentional design tradeoff.
|
@aaight Dead code left behind: The old trigger handlers src/triggers/trello/card-moved.ts and src/triggers/jira/issue-transitioned.ts still exist on disk but are no longer imported by any production code (register files, barrel exports, etc. all reference the new status-changed.ts files). Their corresponding test files (tests/unit/triggers/card-moved.test.ts, tests/unit/triggers/jira-issue-transitioned.test.ts) are also orphaned. These should be deleted in this PR to avoid confusion — someone browsing the codebase would see both the old and new triggers side by side with no clear indication that the old ones are dead. |
Request AddressedWhat was askedDelete dead code files left behind after the trigger unification refactor: old trigger handlers ( Changes Made
Verification
|
Delete old trigger handlers and their orphaned tests that were replaced by the unified status-changed triggers but left on disk. Removed files: - src/triggers/trello/card-moved.ts - src/triggers/jira/issue-transitioned.ts - tests/unit/triggers/card-moved.test.ts - tests/unit/triggers/jira-issue-transitioned.test.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done! Deleted all 4 dead code files in commit 8c868e3:
Verified before pushing:
|
|
✅ Review complete for feature/unify-pm-status-changed-trigger Verdict: REQUEST_CHANGES — two backward-compatibility issues with Trello config Findings
What's good
|
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean unification of PM triggers — good direction. Two concerns: (1) Trello loses per-agent granularity at the integration config level, and (2) legacy Trello config keys (cardMovedToTodo: false) silently become no-ops since the trigger handler now only checks statusChanged.
Architecture & Design
The consolidation of pm:card-moved and pm:issue-transitioned into pm:status-changed is a sound architectural improvement — it removes provider-specific leakage from the trigger event namespace and aligns with the existing pattern where pm:label-added and pm:comment-mention are already provider-agnostic. The TRIGGER_REGISTRY simplification from 3 PM entries to 1 unified entry is clean.
Code Issues
Should Fix
-
Trello per-agent granularity loss (
src/triggers/trello/status-changed.ts): Previously, the three Trello card-moved triggers used distinct config keys (cardMovedToSplitting,cardMovedToPlanning,cardMovedToTodo), allowing operators to disable e.g. only the splitting trigger via the integration config. Now all three sharetriggerConfigKey: 'statusChanged', sostatusChanged: falsekills all three at once. Theagent_trigger_configstable still allows per-agent toggles, but the integration-level config loses granularity. Jira'sstatusChangedsupports per-agent objects ({ splitting: true, planning: false }), but Trello'sstatusChangedis a barez.boolean(). Consider either: (a) making Trello'sstatusChangedalso accept a per-agent object (matching Jira'sStatusChangedSchema), or (b) documenting this as an intentional simplification. -
Legacy Trello config keys are silently dead (
src/config/triggerConfig.ts): ThecardMovedToSplitting,cardMovedToPlanning,cardMovedToTodokeys still exist in the Zod schema (they parse without error), but the trigger handler now exclusively checksstatusChanged. This means an existing config like{ cardMovedToTodo: false }will parse fine, appear to be valid, but will not actually disable the trigger. This is a subtle backward compatibility break. Options: (a) wire the legacy keys intoresolveTrelloTriggerEnabledas fallbacks (similar to how Jira'sresolveStatusChangedEnabledfalls back toissueTransitioned), or (b) remove the legacy keys from the schema entirely so invalid configs fail loudly at parse time.
Questions
agent_trigger_configstable migration: The migration tool (tools/migrate-triggers.ts) correctly maps legacyproject_integrations.triggersJSONB keys topm:status-changed. However, if any existingagent_trigger_configsrows already havetriggerEvent = 'pm:card-moved'ortriggerEvent = 'pm:issue-transitioned'(e.g., set via the dashboard/CLI before this PR), those rows will no longer match the new trigger event name. Theconfig-resolver.tsmatches on exacttriggerEventstring. Is there a plan to update those existing rows, or is it expected that the migration tool handles this (it currently only inserts new rows, skipping existing ones)?
| listKey: 'splitting', | ||
| agentType: 'splitting', | ||
| triggerConfigKey: 'cardMovedToSplitting', | ||
| triggerConfigKey: 'statusChanged', |
There was a problem hiding this comment.
[SHOULD_FIX] All three Trello triggers now share triggerConfigKey: 'statusChanged', which means statusChanged: false disables all three at once. Previously, each had its own key (cardMovedToSplitting, cardMovedToPlanning, cardMovedToTodo) giving per-agent control at the integration config level.
Jira's statusChanged already supports per-agent objects via StatusChangedSchema. Consider using the same schema for Trello:
statusChanged: StatusChangedSchema, // instead of z.boolean().default(true)Then the factory could use a per-agent triggerConfigKey like splitting / planning / todo and resolve via resolvePerAgentToggle.
There was a problem hiding this comment.
Fixed! Changed Trello's statusChanged to use StatusChangedSchema (same as Jira), enabling per-agent granularity. Now supports both boolean and per-agent object forms like { splitting: true, planning: false, implementation: true }.
| */ | ||
| export const TrelloTriggerConfigSchema = z.object({ | ||
| /** Unified status-changed toggle (replaces legacy cardMovedTo* keys). */ | ||
| statusChanged: z.boolean().default(true), |
There was a problem hiding this comment.
[SHOULD_FIX] This is a bare z.boolean(), but the trigger handler now exclusively checks this key, making the legacy cardMovedToSplitting/cardMovedToPlanning/cardMovedToTodo keys below dead config — they parse without error but have no effect.
Two options:
- Add fallback logic in
resolveTrelloTriggerEnabled(analogous to howresolveStatusChangedEnabledfalls back fromstatusChangedtoissueTransitionedfor Jira) - Remove the legacy keys from the schema so stale configs fail loudly at parse time
Option 1 is safer for backward compatibility.
There was a problem hiding this comment.
Fixed! Added resolveTrelloStatusChangedEnabled() with fallback pattern matching Jira's resolveStatusChangedEnabled(). Now falls back to cardMovedToSplitting/cardMovedToPlanning/cardMovedToTodo when statusChanged is not set, so legacy keys work properly instead of being silently ignored.
| * Supports both the new nested object format and the legacy boolean format. | ||
| * Returns `true` when no config is present (backward compatible). | ||
| */ | ||
| export function resolveStatusChangedEnabled( |
There was a problem hiding this comment.
[NIT] Good fallback pattern — statusChanged takes precedence, issueTransitioned as fallback. Note that the Trello side lacks an equivalent fallback for its legacy keys (cardMovedToSplitting, etc.) — see comment on the Trello schema above.
There was a problem hiding this comment.
Thanks for the observation! I've now implemented the same fallback pattern for Trello. The resolveTrelloStatusChangedEnabled() function now mirrors the Jira pattern, checking statusChanged first and falling back to the legacy per-agent keys (cardMovedToSplitting/Planning/Todo).
Review Feedback AddressedChanges MadeIssue 1: Trello lost per-agent granularity
Issue 2: Legacy Trello config keys were silently dead
Testing
Verification
|
…lo status-changed Address review feedback on PR #605: Issue 1: Trello triggers now support per-agent granularity - Changed TrelloTriggerConfigSchema.statusChanged from z.boolean() to StatusChangedSchema - This allows statusChanged to accept both boolean and per-agent objects like Jira - Config examples: statusChanged: false (all agents) or { splitting: true, planning: false, implementation: true } Issue 2: Legacy Trello config keys now work via fallback - Added resolveTrelloStatusChangedEnabled() function with fallback pattern like Jira's resolveStatusChangedEnabled() - Falls back to cardMovedToSplitting/cardMovedToPlanning/cardMovedToTodo when statusChanged not set - Trigger handlers now use resolveTrelloStatusChangedEnabled() instead of resolveTrelloTriggerEnabled() - Legacy keys are no longer silently dead - they work when statusChanged is absent Added comprehensive tests for new per-agent and fallback behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
✨ On it — checking the feature/unify-pm-status-changed-trigger branch Progress: [█░░░░░░░░░] 7% (iteration 5/70) 🔍 Code Review Update (1 min) I've completed the initial context gathering, reviewing the provided PR details, diff, CI checks, and relevant file contents. My current focus is on strategically analyzing the PR, which appears to unify two existing functionalities. I'm now systematically analyzing the changes to ensure quality and correctness. Last updated: iteration 5 · review |
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-executed unification of provider-specific PM trigger names into a single pm:status-changed event. LGTM.
Details
The change correctly abstracts away the provider distinction at the trigger event level — Trello's pm:card-moved and Jira's pm:issue-transitioned are unified under pm:status-changed. This is the right call: the trigger registry shouldn't leak provider semantics into agent definitions.
Key things verified:
- No stale references: The only remaining mention of old event names (
pm:card-moved,pm:issue-transitioned) is in a documentation comment inschema.tsthat explains the replacement — correct. - Backward compatibility: Legacy config keys (
cardMovedToSplitting,cardMovedToPlanning,cardMovedToTodofor Trello;issueTransitionedfor Jira) are properly handled via fallback logic inresolveTrelloStatusChangedEnabled()andresolveStatusChangedEnabled(). Deprecated aliases are kept for the schema and resolver functions. - TRIGGER_REGISTRY: Consolidated from two provider-restricted entries to one provider-agnostic entry — correct since the provider distinction is now handled at the handler level, not the event level.
- Type relocation:
TrelloWebhookPayloadmoved tosrc/triggers/trello/types.tswith re-exports from the original location — no breakage for existing consumers. - Agent YAML definitions: All three agents (
splitting,planning,implementation) correctly reference the singlepm:status-changedevent withtargetStatusparameter (renamed fromtargetList). - Migration tool: Properly maps both legacy event names to the new unified name.
- All CI checks pass.
nhopeatall
left a comment
There was a problem hiding this comment.
LGTM - Clean unification of provider-specific PM triggers into pm:status-changed. Backward compatibility properly maintained via legacy key fallbacks. All CI checks pass.
Summary
Consolidates
pm:card-moved(Trello) andpm:issue-transitioned(Jira) into a unifiedpm:status-changedtrigger at the category level.Card: https://trello.com/c/69a57053927fc3eebd84b5d5
pm:status-changedentry (no provider restriction)pm:status-changedtrigger per agent (splitting, planning, implementation) replacing two provider-specific onesTrelloStatusChangedTriggerandJiraStatusChangedTriggerTrelloWebhookPayloadmoved tosrc/triggers/trello/types.ts(re-exported for compat)statusChangedkey; legacycardMovedTo*/issueTransitionedstill parse correctlyresolveStatusChangedEnabled: PrefersstatusChangedkey, falls back toissueTransitionedfor backward compatpm:status-changedstatus-changed.test.tsandjira-status-changed.test.tsTest plan
issueTransitionedconfig key respected viaresolveStatusChangedEnabledfallback🤖 Generated with Claude Code