fix(triggers): address code review findings for integration-driven architecture#596
Merged
zbigniewsobiecki merged 2 commits intodevfrom Mar 1, 2026
Merged
Conversation
…chitecture
Schema validation improvements:
- Add TriggerEventSchema with regex validation for {category}:{event-name} format
- Add KnownProviderSchema enum (trello, jira, github, imap, gmail, twilio)
- Add defaultValue type consistency refinement (must match parameter type)
- Add refinement preventing required: true with defaultValue
- Add IntegrationRequirementsSchema overlap refinement
Repository fixes:
- Fix bulkUpsertTriggerConfigs bug (was using first config's values for all conflicts)
- Add getTriggerConfigById function for ownership verification
- Use individual upserts in transaction for correct per-config values
Router refactoring:
- Replace direct DB access with repository functions in update/delete procedures
- Clean up imports (remove unused getDb, eq)
YAML definition fixes:
- review.yaml: consolidate duplicate scm:check-suite-success triggers into single
trigger with authorMode select parameter (own/external/all)
- debug.yaml: remove undeclared pm:attachment-added trigger (agent is manually triggered)
- implementation.yaml, planning.yaml, splitting.yaml: remove contradictory
required: true from parameters that have defaultValue
Tests:
- Add comprehensive tests for TriggerParameterSchema validation
- Add tests for SupportedTriggerSchema event format validation
- Add tests for KnownProviderSchema
- Add tests for IntegrationRequirementsSchema overlap
- Add tests for AgentDefinitionSchema with triggers
- Add full test suite for agentTriggerConfigs router (51 new tests)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add `triggers: []` to EMPTY_DEFINITION in agent-definition-editor.tsx to satisfy the TypeScript type now that triggers is a required field in AgentDefinition. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
bulkUpsertTriggerConfigsbug (was using first config's values for all conflicts)Schema Validation Improvements
{category}:{event-name}(e.g.,pm:card-moved,scm:check-suite-success)required: truewithdefaultValueRepository Fixes
bulkUpsertTriggerConfigs: Now uses individual upserts in a transaction to ensure each config's values are used correctlygetTriggerConfigByIdfor ownership verification in routerYAML Fixes
scm:check-suite-successtriggers into single trigger withauthorModeselect parameterpm:attachment-addedtrigger (agent is manually triggered)required: truefrom parameters withdefaultValueTest Plan
🤖 Generated with Claude Code