From 04d3bcd9a74333b8fdf5e0880f10561977a23ec0 Mon Sep 17 00:00:00 2001 From: Zbigniew Sobiecki Date: Tue, 24 Mar 2026 11:59:45 +0000 Subject: [PATCH] fix(backlog-manager): fix false "Backlog Blocked" when dependencies are already merged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Root cause The backlog-manager declared the backlog blocked in two situations: 1. **LLM matching failure**: haiku couldn't match a short dependency reference ("SCMIntegration") in a backlog card's description to the full user-story title in the MERGED list ("As a developer, I want SCMIntegration and GitHubSCMIntegration…"). Stale "(not yet merged)" annotations in descriptions further anchored it toward "blocked". 2. **Missing re-trigger**: when blocking dependency cards were manually moved to MERGED in Trello (outside CASCADE's PR flow), no trigger fired, leaving the backlog incorrectly blocked indefinitely. ## Changes **Prompt improvements** (`backlog-manager.eta`): - Add explicit warning that "(not yet merged)" text is always stale — the MERGED list is the only source of truth - Replace vague MERGED-check guidance with concrete substring/keyword matching instructions: if the dependency name appears anywhere in a MERGED title it is resolved - Scope the conservative rule: be conservative when *detecting* dependencies, but generous when *resolving* them against MERGED **Pipeline snapshot** (`contextSteps.ts`): - Include URL in parentheses for DONE/MERGED items so the agent can do exact URL matching when descriptions reference PM links - Guard against empty URL (render nothing rather than empty `()`) **New trigger** (`status-changed.ts`, `register.ts`): - Add `TrelloStatusChangedMergedTrigger`: re-triggers backlog-manager whenever any card is moved to the MERGED list, catching manually resolved dependencies that would otherwise leave the backlog stuck **Agent definition** (`backlog-manager.yaml`): - Remove misleading `targetStatus` parameter that implied independent control over backlog vs. merged firing (both always fire when enabled) - Update label and description to accurately describe dual-list behaviour **Tests**: - Add `merged-status-changed.test.ts` with full parallel coverage to `backlog-status-changed.test.ts` (matches, non-matches, disabled trigger, handle result) - Update pipeline snapshot test: rename "title-only" → "title-and-url", assert URL present, add empty-URL edge-case test - Add missing `triggerEvent` assertion to backlog trigger test Co-Authored-By: Claude Sonnet 4.6 --- src/agents/definitions/backlog-manager.yaml | 14 +- src/agents/definitions/contextSteps.ts | 2 +- .../prompts/templates/backlog-manager.eta | 8 +- src/triggers/trello/register.ts | 2 + src/triggers/trello/status-changed.ts | 10 +- .../definitions/pipelineSnapshot.test.ts | 21 +- .../triggers/backlog-status-changed.test.ts | 1 + tests/unit/triggers/builtins.test.ts | 8 +- .../triggers/merged-status-changed.test.ts | 251 ++++++++++++++++++ 9 files changed, 299 insertions(+), 18 deletions(-) create mode 100644 tests/unit/triggers/merged-status-changed.test.ts diff --git a/src/agents/definitions/backlog-manager.yaml b/src/agents/definitions/backlog-manager.yaml index f1d0ada4..6b24a6a2 100644 --- a/src/agents/definitions/backlog-manager.yaml +++ b/src/agents/definitions/backlog-manager.yaml @@ -29,15 +29,13 @@ triggers: defaultEnabled: false contextPipeline: [pipelineSnapshot] - event: pm:status-changed - label: Item Added to Backlog - description: When an item is moved to Backlog, check if pipeline is empty and it should be moved to TODO immediately + label: Backlog / Merged Card Move + description: > + Triggers when a card is moved to the Backlog list (new work available) OR to the + Merged list (a blocking dependency was resolved). Both cases may allow the next + backlog item to be pulled into TODO. Note: when enabled, this fires for both + list moves — they cannot be independently toggled. defaultEnabled: false - parameters: - - name: targetStatus - type: select - label: Target Status - options: [backlog] - defaultValue: backlog contextPipeline: [pipelineSnapshot] - event: internal:auto-chain label: Auto-chain after Splitting diff --git a/src/agents/definitions/contextSteps.ts b/src/agents/definitions/contextSteps.ts index ba74debe..4f11ba94 100644 --- a/src/agents/definitions/contextSteps.ts +++ b/src/agents/definitions/contextSteps.ts @@ -486,7 +486,7 @@ function appendPipelineSection( if (!PIPELINE_DETAIL_LISTS.has(list.name)) { for (const item of items) { - sections.push(`- [${item.id}] ${item.title}`); + sections.push(`- [${item.id}] ${item.title}${item.url ? ` (${item.url})` : ''}`); } sections.push(''); return; diff --git a/src/agents/prompts/templates/backlog-manager.eta b/src/agents/prompts/templates/backlog-manager.eta index 73dd3979..d31c80e8 100644 --- a/src/agents/prompts/templates/backlog-manager.eta +++ b/src/agents/prompts/templates/backlog-manager.eta @@ -36,7 +36,7 @@ A **Pipeline Snapshot** has been pre-loaded into your context containing the cur 3. **Only if the active pipeline count is below the capacity limit**, proceed to backlog selection. The number of <%= it.workItemNounPlural || 'cards' %> you may move = capacity limit (<%= it.maxInFlightItems ?? 1 %>) minus current active count. -Note: DONE and MERGED <%= it.workItemNounPlural || 'cards' %> are completed work and do not block new work from being selected. The snapshot shows their titles for dependency checking. +Note: DONE and MERGED <%= it.workItemNounPlural || 'cards' %> are completed work and do not block new work from being selected. The snapshot shows their titles and URLs for dependency checking. ## Backlog Selection Process @@ -51,7 +51,8 @@ When the active pipeline has capacity: - References to other <%= it.workItemNounPlural || 'cards' %>: "blocked by", "depends on", "waiting for", "after" - Cross-references to <%= it.workItemNoun || 'card' %> IDs, URLs, or titles - Comments indicating external dependencies - - **IMPORTANT**: Before declaring a <%= it.workItemNoun || 'card' %> blocked, check whether the dependency exists in the MERGED list. A dependency in MERGED is **resolved** — it does NOT block. Check the pre-loaded Pipeline Snapshot MERGED section (titles are provided for dependency checking). + - **Stale annotations**: Text like "(not yet merged)" in a description was written when the <%= it.workItemNoun || 'card' %> was created and is **always stale**. Do NOT use it as evidence of blocked status — only the MERGED list itself is authoritative. + - **IMPORTANT — MERGED check**: Before declaring a <%= it.workItemNoun || 'card' %> blocked, scan the MERGED section of the Pipeline Snapshot. Use **substring matching**: if the dependency name (e.g., "SCMIntegration", "OpenCodeEngine", "integrationRoles") appears anywhere within a MERGED title, that dependency is **resolved** and does NOT block. Each MERGED entry also shows its URL in parentheses — if the description references a <%= it.pmName || 'PM' %> link, match it against the URL too. A module or class name found anywhere in a title counts as a match. 4. **Select the best unblocked <%= it.workItemNoun || 'card' %>(s)** considering: - Smaller, self-contained <%= it.workItemNounPlural || 'cards' %> are preferred - <%= it.workItemNounPluralCap || 'Cards' %> with clear acceptance criteria @@ -101,6 +102,7 @@ Manual intervention may be needed to unblock the backlog. - ALWAYS read <%= it.workItemNoun || 'card' %> contents before making a selection decision - <%= it.maxInFlightItems == null || it.maxInFlightItems === 1 ? 'ALWAYS move exactly ONE ' + (it.workItemNoun || 'card') + ' per run' : 'Move only as many ' + (it.workItemNounPlural || 'cards') + ' as needed to reach capacity (limit: ' + it.maxInFlightItems + ')' %> - ALWAYS post a comment BEFORE moving the <%= it.workItemNoun || 'card' %> — comment first, then move to TODO -- BE CONSERVATIVE with dependency detection - when unsure, treat as blocked +- CONSERVATIVE on detecting dependencies — when unsure if text implies a dependency, treat it as one. But GENEROUS on MERGED resolution — use substring matching and prefer resolved over blocked for ambiguous matches. - LOOK FOR dependency keywords: "blocked by", "depends on", "waiting for", "after", "requires" +- IGNORE "(not yet merged)" and similar stale annotations in descriptions — they are written at card creation and never updated. The MERGED list is the only source of truth. - EXECUTE COMMANDS — DO NOT JUST DESCRIBE THEM: When you decide to post a comment or move a card, you MUST actually invoke the command as a tool call. Writing a command inside a code block without invoking it does NOT execute it — text output has no effect on the system. If you find yourself writing out a command without calling it, stop and call it instead. diff --git a/src/triggers/trello/register.ts b/src/triggers/trello/register.ts index 04ba7960..938d7cd4 100644 --- a/src/triggers/trello/register.ts +++ b/src/triggers/trello/register.ts @@ -14,6 +14,7 @@ import { TrelloCommentMentionTrigger } from './comment-mention.js'; import { ReadyToProcessLabelTrigger } from './label-added.js'; import { TrelloStatusChangedBacklogTrigger, + TrelloStatusChangedMergedTrigger, TrelloStatusChangedPlanningTrigger, TrelloStatusChangedSplittingTrigger, TrelloStatusChangedTodoTrigger, @@ -33,6 +34,7 @@ export function registerTrelloTriggers(registry: TriggerRegistry): void { registry.register(TrelloStatusChangedPlanningTrigger); registry.register(TrelloStatusChangedTodoTrigger); registry.register(TrelloStatusChangedBacklogTrigger); + registry.register(TrelloStatusChangedMergedTrigger); registry.register(new ReadyToProcessLabelTrigger()); } diff --git a/src/triggers/trello/status-changed.ts b/src/triggers/trello/status-changed.ts index 0dee7f7f..2d928404 100644 --- a/src/triggers/trello/status-changed.ts +++ b/src/triggers/trello/status-changed.ts @@ -11,7 +11,7 @@ import { type TrelloWebhookPayload, isTrelloWebhookPayload } from './types.js'; interface StatusChangedConfig { name: string; description: string; - listKey: 'splitting' | 'planning' | 'todo' | 'backlog'; + listKey: 'splitting' | 'planning' | 'todo' | 'backlog' | 'merged'; agentType: 'splitting' | 'planning' | 'implementation' | 'backlog-manager'; } @@ -115,3 +115,11 @@ export const TrelloStatusChangedBacklogTrigger = createStatusChangedTrigger({ listKey: 'backlog', agentType: 'backlog-manager', }); + +export const TrelloStatusChangedMergedTrigger = createStatusChangedTrigger({ + name: 'trello-status-changed-merged', + description: + 'Re-triggers backlog-manager when any card is moved to MERGED, so manually resolved dependencies unblock the backlog', + listKey: 'merged', + agentType: 'backlog-manager', +}); diff --git a/tests/unit/agents/definitions/pipelineSnapshot.test.ts b/tests/unit/agents/definitions/pipelineSnapshot.test.ts index 962b38d6..cf31b457 100644 --- a/tests/unit/agents/definitions/pipelineSnapshot.test.ts +++ b/tests/unit/agents/definitions/pipelineSnapshot.test.ts @@ -145,7 +145,7 @@ describe('fetchPipelineSnapshotStep', () => { expect(output).toContain('Full details here'); }); - it('uses title-only format for DONE and MERGED lists', async () => { + it('uses title-and-url format for DONE and MERGED lists', async () => { mockGetPMProviderOrNull.mockReturnValue(mockProvider as never); const card = { id: 'card-done', title: 'Done Card', url: 'http://trello.com/c/2', labels: [] }; @@ -161,8 +161,25 @@ describe('fetchPipelineSnapshotStep', () => { expect(mockReadWorkItem).not.toHaveBeenCalledWith('card-done', true); const output = result[0].result as string; - // Title-only format + // Title + URL format expect(output).toContain('[card-done] Done Card'); + expect(output).toContain('http://trello.com/c/2'); + }); + + it('omits URL parentheses for DONE/MERGED items when url is empty', async () => { + mockGetPMProviderOrNull.mockReturnValue(mockProvider as never); + + const card = { id: 'card-done', title: 'Done Card', url: '', labels: [] }; + mockProvider.listWorkItems.mockImplementation(async (listId: string) => { + if (listId === 'list-done') return [card]; + return []; + }); + + const result = await fetchPipelineSnapshotStep(makeParams({}, makeProject())); + + const output = result[0].result as string; + expect(output).toContain('[card-done] Done Card'); + expect(output).not.toContain('()'); }); it('handles list fetch errors gracefully', async () => { diff --git a/tests/unit/triggers/backlog-status-changed.test.ts b/tests/unit/triggers/backlog-status-changed.test.ts index 0cc965e6..559c28ff 100644 --- a/tests/unit/triggers/backlog-status-changed.test.ts +++ b/tests/unit/triggers/backlog-status-changed.test.ts @@ -190,6 +190,7 @@ describe('TrelloStatusChangedBacklogTrigger', () => { expect(result?.agentType).toBe('backlog-manager'); expect(result?.workItemId).toBe('card789'); expect(result?.agentInput.workItemId).toBe('card789'); + expect(result?.agentInput.triggerEvent).toBe('pm:status-changed'); }); it('returns null when card ID is missing from payload', async () => { diff --git a/tests/unit/triggers/builtins.test.ts b/tests/unit/triggers/builtins.test.ts index 3899ca4e..8a200107 100644 --- a/tests/unit/triggers/builtins.test.ts +++ b/tests/unit/triggers/builtins.test.ts @@ -42,6 +42,7 @@ vi.mock('../../../src/triggers/trello/status-changed.js', () => ({ TrelloStatusChangedPlanningTrigger: { name: 'trello-status-changed-planning' }, TrelloStatusChangedTodoTrigger: { name: 'trello-status-changed-todo' }, TrelloStatusChangedBacklogTrigger: { name: 'trello-status-changed-backlog' }, + TrelloStatusChangedMergedTrigger: { name: 'trello-status-changed-merged' }, })); vi.mock('../../../src/triggers/trello/comment-mention.js', () => ({ TrelloCommentMentionTrigger: vi @@ -87,8 +88,8 @@ describe('registerBuiltInTriggers', () => { registerBuiltInTriggers(registry as unknown as TriggerRegistry); - // Should have registered all 20 built-in triggers (18 + 2 Sentry alerting triggers) - expect(registry.register).toHaveBeenCalledTimes(20); + // Should have registered all 21 built-in triggers (19 + 2 Sentry alerting triggers) + expect(registry.register).toHaveBeenCalledTimes(21); }); it('registers TrelloCommentMentionTrigger first', () => { @@ -100,7 +101,7 @@ describe('registerBuiltInTriggers', () => { expect(firstCall.name).toBe('trello-comment-mention'); }); - it('registers all four status-changed triggers (Trello)', () => { + it('registers all five status-changed triggers (Trello)', () => { const registry = createMockRegistry(); registerBuiltInTriggers(registry as unknown as TriggerRegistry); @@ -110,6 +111,7 @@ describe('registerBuiltInTriggers', () => { expect(registeredNames).toContain('trello-status-changed-planning'); expect(registeredNames).toContain('trello-status-changed-todo'); expect(registeredNames).toContain('trello-status-changed-backlog'); + expect(registeredNames).toContain('trello-status-changed-merged'); }); it('registers GitHub triggers', () => { diff --git a/tests/unit/triggers/merged-status-changed.test.ts b/tests/unit/triggers/merged-status-changed.test.ts new file mode 100644 index 00000000..c0eb53b7 --- /dev/null +++ b/tests/unit/triggers/merged-status-changed.test.ts @@ -0,0 +1,251 @@ +import { describe, expect, it, vi } from 'vitest'; +import { + mockAcknowledgmentsModule, + mockConfigProvider, + mockConfigResolverModule, + mockJiraClientModule, + mockLogger, + mockReactionsModule, + mockTrelloClientModule, + mockTriggerCheckModule, +} from '../../helpers/sharedMocks.js'; + +vi.mock('../../../src/utils/logging.js', () => ({ logger: mockLogger })); + +vi.mock('../../../src/triggers/config-resolver.js', () => mockConfigResolverModule); +vi.mock('../../../src/triggers/shared/trigger-check.js', () => mockTriggerCheckModule); + +// Mocks required for PM integration registration (pm/index.js side-effect) +vi.mock('../../../src/config/provider.js', () => mockConfigProvider); +vi.mock('../../../src/trello/client.js', () => mockTrelloClientModule); +vi.mock('../../../src/jira/client.js', () => mockJiraClientModule); +vi.mock('../../../src/router/acknowledgments.js', () => mockAcknowledgmentsModule); +vi.mock('../../../src/router/reactions.js', () => mockReactionsModule); + +// Register PM integrations in the registry +import '../../../src/pm/index.js'; + +import { checkTriggerEnabled } from '../../../src/triggers/shared/trigger-check.js'; +import { TrelloStatusChangedMergedTrigger } from '../../../src/triggers/trello/status-changed.js'; +import type { TriggerContext } from '../../../src/triggers/types.js'; +import { createMockProject, createTrelloActionPayload } from '../../helpers/factories.js'; + +describe('TrelloStatusChangedMergedTrigger', () => { + const trigger = TrelloStatusChangedMergedTrigger; + + const mockProject = createMockProject({ + trello: { + boardId: 'board123', + lists: { + splitting: 'splitting-list-id', + planning: 'planning-list-id', + todo: 'todo-list-id', + backlog: 'backlog-list-id', + merged: 'merged-list-id', + }, + labels: {}, + }, + }); + + it('matches when card moved to merged list', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'in-review-list', name: 'In Review' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + expect(trigger.matches(ctx)).toBe(true); + }); + + it('does not match when card moved from merged to merged', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'merged-list-id', name: 'Merged' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + expect(trigger.matches(ctx)).toBe(false); + }); + + it('matches when card created directly in merged list', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'createCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + list: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + expect(trigger.matches(ctx)).toBe(true); + }); + + it('does not match when card moved to a different list', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'other-list', name: 'Other' }, + listAfter: { id: 'todo-list-id', name: 'TODO' }, + }, + }, + }), + }; + + expect(trigger.matches(ctx)).toBe(false); + }); + + it('does not match when merged list is not configured', () => { + const projectWithoutMerged = createMockProject(); + const ctx: TriggerContext = { + project: projectWithoutMerged, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'other-list', name: 'Other' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + // merged list not configured, so targetListId is undefined — won't match + expect(trigger.matches(ctx)).toBe(false); + }); + + it('does not match github source', () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'github', + payload: {}, + }; + + expect(trigger.matches(ctx)).toBe(false); + }); + + it('handles and returns backlog-manager agent with pm:status-changed event', async () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card789', name: 'Resolved Dependency', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'in-review-list', name: 'In Review' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + const result = await trigger.handle(ctx); + + expect(result?.agentType).toBe('backlog-manager'); + expect(result?.workItemId).toBe('card789'); + expect(result?.agentInput.workItemId).toBe('card789'); + expect(result?.agentInput.triggerEvent).toBe('pm:status-changed'); + }); + + it('returns null when trigger is disabled', async () => { + vi.mocked(checkTriggerEnabled).mockResolvedValueOnce(false); + + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + card: { id: 'card1', name: 'Test Card', idShort: 1, shortLink: 'abc' }, + listBefore: { id: 'in-review-list', name: 'In Review' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + const result = await trigger.handle(ctx); + expect(result).toBeNull(); + expect(checkTriggerEnabled).toHaveBeenCalledWith( + mockProject.id, + 'backlog-manager', + 'pm:status-changed', + 'trello-status-changed-merged', + ); + }); + + it('returns null when card ID is missing from payload', async () => { + const ctx: TriggerContext = { + project: mockProject, + source: 'trello', + payload: createTrelloActionPayload({ + action: { + id: 'action1', + idMemberCreator: 'member1', + type: 'updateCard', + date: '2024-01-01', + data: { + // No card field + listBefore: { id: 'other-list', name: 'Other' }, + listAfter: { id: 'merged-list-id', name: 'Merged' }, + }, + }, + }), + }; + + const result = await trigger.handle(ctx); + expect(result).toBeNull(); + }); +});