From e92af8bb0ea62cd7c0413c6b77bba9e75aa2b0d8 Mon Sep 17 00:00:00 2001 From: laithalenooz Date: Wed, 7 Oct 2026 16:17:27 +0300 Subject: [PATCH] fix(automation): fire tag_added only when the tag is actually added (#554) The tag_added trigger matched whenever the configured tag was present on the task, and every update to the tags field produced a tag_added candidate. Any later, unrelated tag edit (or a removal) therefore re-fired automations whose tag had been added long before, which in multi-stage review pipelines made tasks bounce back to earlier stages and started duplicate agent runs. The consumer now derives the tags an update added from the change's old and new values and passes them to trigger matching: a configured tag must be among them (and still present on the task), and an unconfigured tag_added trigger needs at least one addition. Removal-only edits no longer produce a tag_added candidate. Fixes #554 --- .../internal/worker/automation_consumer.go | 47 +++++++++++++--- .../worker/automation_consumer_test.go | 55 ++++++++++++++++--- 2 files changed, 87 insertions(+), 15 deletions(-) diff --git a/services/api/internal/worker/automation_consumer.go b/services/api/internal/worker/automation_consumer.go index c4070bfc5..a373bb213 100644 --- a/services/api/internal/worker/automation_consumer.go +++ b/services/api/internal/worker/automation_consumer.go @@ -481,6 +481,7 @@ func (c *AutomationConsumer) handle(msg redis.XMessage) { } var candidates []automationdom.TriggerType + var added []string checkPredecessors := false switch p.ActivityType { @@ -499,7 +500,11 @@ func (c *AutomationConsumer) handle(msg redis.XMessage) { case "importance": candidates = append(candidates, automationdom.TriggerPriorityChanged) case "tags": - candidates = append(candidates, automationdom.TriggerTagAdded) + // Only an actual addition is a tag_added event: removals and + // edits that keep an existing tag must not re-fire it (#554). + if added = addedTags(change); len(added) > 0 { + candidates = append(candidates, automationdom.TriggerTagAdded) + } } } } @@ -510,7 +515,7 @@ func (c *AutomationConsumer) handle(msg redis.XMessage) { return } - if err := c.processEvent(ctx, projectID, taskID, candidates, checkPredecessors); err != nil { + if err := c.processEvent(ctx, projectID, taskID, candidates, checkPredecessors, added); err != nil { c.log.Error("automation consumer: failed to process event", "id", msg.ID, "task_id", taskID, "err", err) // Do not ack — retried via processPending on next restart. return @@ -760,7 +765,7 @@ func (c *AutomationConsumer) handleSprintActivity(msg redis.XMessage) { // automation are logged and skipped (best-effort) rather than aborting the // whole batch. An error is returned only when the foundational task lookup // fails, so the message is retried. -func (c *AutomationConsumer) processEvent(ctx context.Context, projectID, taskID uuid.UUID, candidates []automationdom.TriggerType, checkPredecessors bool) error { +func (c *AutomationConsumer) processEvent(ctx context.Context, projectID, taskID uuid.UUID, candidates []automationdom.TriggerType, checkPredecessors bool, added []string) error { task, err := c.taskRepo.FindTaskByID(ctx, taskID) if err != nil { return fmt.Errorf("find task: %w", err) @@ -773,7 +778,7 @@ func (c *AutomationConsumer) processEvent(ctx context.Context, projectID, taskID continue } for _, node := range nodes { - if !c.triggerMatches(node, task) { + if !c.triggerMatches(node, task, added) { continue } if err := c.executeRun(ctx, projectID, node, task); err != nil { @@ -822,8 +827,9 @@ func (c *AutomationConsumer) processEvent(ctx context.Context, projectID, taskID // triggerMatches checks a trigger node's config filter against the task's // current authoritative state (not the raw activity payload — the payload -// only carries resolved names/diffs for some fields). -func (c *AutomationConsumer) triggerMatches(node *automationdom.Node, task *taskdom.Task) bool { +// only carries resolved names/diffs for some fields). added lists the tags the +// triggering update introduced; tag_added matches only those. +func (c *AutomationConsumer) triggerMatches(node *automationdom.Node, task *taskdom.Task, added []string) bool { var cfg automationdom.TriggerConfig if len(node.Config) > 0 { _ = json.Unmarshal(node.Config, &cfg) @@ -835,10 +841,12 @@ func (c *AutomationConsumer) triggerMatches(node *automationdom.Node, task *task } return task.StatusID != nil && *task.StatusID == *cfg.StatusID case automationdom.TriggerTagAdded: + // added holds the tags this update introduced; the task's current tags + // confirm the tag is still there by the time the event is processed. if cfg.Tag == "" { - return true + return len(added) > 0 } - return slices.Contains(task.Tags, cfg.Tag) + return slices.Contains(added, cfg.Tag) && slices.Contains(task.Tags, cfg.Tag) case automationdom.TriggerTaskCreated, automationdom.TriggerAssigneeChanged, automationdom.TriggerPriorityChanged: return true default: @@ -2626,3 +2634,26 @@ func (c *AutomationConsumer) recordAppliedActivity(ctx context.Context, projectI Content: content, }) } + +// addedTags returns the tags present in change.New but not in change.Old, in +// the order they appear in New. Old and New arrive JSON-decoded ([]any), as +// recorded by the task handler and the autofill consumer. +func addedTags(change taskdom.FieldChange) []string { + before := map[string]bool{} + if old, ok := change.Old.([]any); ok { + for _, v := range old { + if s, ok := v.(string); ok { + before[s] = true + } + } + } + var added []string + if next, ok := change.New.([]any); ok { + for _, v := range next { + if s, ok := v.(string); ok && !before[s] { + added = append(added, s) + } + } + } + return added +} diff --git a/services/api/internal/worker/automation_consumer_test.go b/services/api/internal/worker/automation_consumer_test.go index 095b306c3..e7db64b82 100644 --- a/services/api/internal/worker/automation_consumer_test.go +++ b/services/api/internal/worker/automation_consumer_test.go @@ -48,7 +48,7 @@ func TestTriggerMatches_StatusChanged_AnyStatusWhenConfigEmpty(t *testing.T) { statusID := uuid.New() node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(automationdom.TriggerStatusChanged), Config: json.RawMessage(`{}`)} task := &taskdom.Task{StatusID: &statusID} - if !c.triggerMatches(node, task) { + if !c.triggerMatches(node, task, nil) { t.Fatal("expected status_changed trigger with no status_id filter to match any status") } } @@ -60,10 +60,10 @@ func TestTriggerMatches_StatusChanged_FiltersToConfiguredStatus(t *testing.T) { cfg, _ := json.Marshal(automationdom.TriggerConfig{StatusID: &wantStatus}) node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(automationdom.TriggerStatusChanged), Config: cfg} - if !c.triggerMatches(node, &taskdom.Task{StatusID: &wantStatus}) { + if !c.triggerMatches(node, &taskdom.Task{StatusID: &wantStatus}, nil) { t.Fatal("expected match when task's status equals the configured status") } - if c.triggerMatches(node, &taskdom.Task{StatusID: &otherStatus}) { + if c.triggerMatches(node, &taskdom.Task{StatusID: &otherStatus}, nil) { t.Fatal("expected no match when task's status differs from the configured status") } } @@ -73,19 +73,60 @@ func TestTriggerMatches_TagAdded_FiltersToConfiguredTag(t *testing.T) { cfg, _ := json.Marshal(automationdom.TriggerConfig{Tag: "urgent"}) node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(automationdom.TriggerTagAdded), Config: cfg} - if !c.triggerMatches(node, &taskdom.Task{Tags: []string{"urgent", "bug"}}) { - t.Fatal("expected match when the configured tag is present") + if !c.triggerMatches(node, &taskdom.Task{Tags: []string{"urgent", "bug"}}, []string{"urgent"}) { + t.Fatal("expected match when the configured tag was added") } - if c.triggerMatches(node, &taskdom.Task{Tags: []string{"bug"}}) { + if c.triggerMatches(node, &taskdom.Task{Tags: []string{"bug"}}, []string{"bug"}) { t.Fatal("expected no match when the configured tag is absent") } } +// Regression for #554: tag_added used to match whenever the configured tag was +// merely present, so any later tag edit on the task re-fired it. +func TestTriggerMatches_TagAdded_OnlyWhenThisUpdateAddedTheTag(t *testing.T) { + c := newTestConsumer(nil) + cfg, _ := json.Marshal(automationdom.TriggerConfig{Tag: "spec-ok"}) + node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(automationdom.TriggerTagAdded), Config: cfg} + task := &taskdom.Task{Tags: []string{"spec-ok", "security-ok"}} + + if c.triggerMatches(node, task, []string{"security-ok"}) { + t.Fatal("expected no match when an unrelated tag was added and the configured tag was already present") + } + if !c.triggerMatches(node, task, []string{"spec-ok"}) { + t.Fatal("expected a match when this update added the configured tag") + } +} + +func TestTriggerMatches_TagAdded_EmptyConfigNeedsAnAddition(t *testing.T) { + c := newTestConsumer(nil) + node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(automationdom.TriggerTagAdded), Config: json.RawMessage(`{}`)} + task := &taskdom.Task{Tags: []string{"bug"}} + + if c.triggerMatches(node, task, nil) { + t.Fatal("expected no match when the update only removed tags") + } + if !c.triggerMatches(node, task, []string{"bug"}) { + t.Fatal("expected a match when the update added any tag") + } +} + +func TestAddedTags_ComparesOldAndNewAfterAJSONRoundTrip(t *testing.T) { + raw, _ := json.Marshal(taskdom.FieldChange{Field: "tags", Old: []string{"a", "b"}, New: []string{"b", "c", "d"}}) + var change taskdom.FieldChange + if err := json.Unmarshal(raw, &change); err != nil { + t.Fatal(err) + } + got := addedTags(change) + if strings.Join(got, ",") != "c,d" { + t.Fatalf("expected [c d], got %v", got) + } +} + func TestTriggerMatches_UnconditionalTypesAlwaysMatch(t *testing.T) { c := newTestConsumer(nil) for _, tt := range []automationdom.TriggerType{automationdom.TriggerTaskCreated, automationdom.TriggerAssigneeChanged, automationdom.TriggerPriorityChanged} { node := &automationdom.Node{Kind: automationdom.KindTrigger, Type: string(tt), Config: json.RawMessage(`{}`)} - if !c.triggerMatches(node, &taskdom.Task{}) { + if !c.triggerMatches(node, &taskdom.Task{}, nil) { t.Errorf("expected trigger type %q to always match", tt) } }