feat(trello): add required label filter for webhook processing#2
feat(trello): add required label filter for webhook processing#2ada-evorada wants to merge 8 commits intoclaude/cranky-johnsonfrom
Conversation
…mmit - Add gitlabOnly: false to webhook CLI test expectations (create/delete) - Add GITHUB_TOKEN_IMPLEMENTER and GITLAB_TOKEN_IMPLEMENTER env var clearing in credential-scoping test beforeEach to prevent cross-test contamination - Add withGitLabToken mock to credential-scoping test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…lnerability Adds an npm override to force axios transitive dependency (from jira.js and trello.js) to >=1.15.0, fixing CVE GHSA-3p68-rc4w-qgx5 (NO_PROXY hostname normalization bypass leading to SSRF). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
CI Failures ResolvedFixes Applied
Root CauseThe Verification
|
suda
left a comment
There was a problem hiding this comment.
Summary
Clean, correct implementation of the required label filter. Logic is placed in the right layer, follows existing patterns throughout, and all CI checks pass.
Code Issues
Should Fix
Unrelated dependency change in the same PR: package.json and package-lock.json include an axios bump (1.13.5 → 1.15.0) and a proxy-from-env major version bump (1.1.0 → 2.1.0), plus a new axios: >=1.15.0 override. This is unrelated to the Trello label filter feature and has no mention in the PR description. It should either be pulled into a separate PR or at minimum documented in the description so reviewers can evaluate it on its own merits.
Minor Observations
-
Filter bypass when
workItemIdis absent: WhenrequiredLabelIdis configured but_event.workItemIdisundefined, the filter is silently skipped and dispatch proceeds. This is intentional defensive behavior (non-card events pass through), but there is no test covering this scenario. A test asserting that dispatch is not skipped (or is skipped with a log) whenworkItemIdis missing would make the intent explicit. -
Error propagation path: If
trelloClient.getCard()throws (transient API failure), the error propagates up towebhook-processor.tswhere it is caught by the existing try/catch and logged as non-fatal. The card already received a 👀 reaction but no ack comment and no job will be queued. This is consistent with the existing error handling pattern across the adapter, so it is not a new problem — just worth knowing as a failure mode.
Everything else looks solid: config propagation is complete across all layers, reducer and buildEditState round-trip correctly, the UI dropdown follows the same FieldMappingRow + manual fallback pattern as the cost field, and the 9 new unit tests cover the meaningful branches.
suda
left a comment
There was a problem hiding this comment.
Despite both cascade and bdgt having set required label in Cascade, moving a Trello card with the label for the bdgt project still was assigned to the cascade project (I removed some sensitive values):
{
"model": {
"id": "69d6891f7325a3eb7e",
"url": "https://trello.com/b/N1R/cascade",
"desc": "",
"name": "Cascade",
"prefs": {
"voting": "disabled",
"canBeOrg": true,
"comments": "members",
"selfJoin": true,
"canInvite": true,
"cardAging": "regular",
"hideVotes": false,
"background": "gradient-bubble",
"cardCounts": false,
"cardCovers": true,
"isTemplate": false,
"autoArchive": null,
"canBePublic": true,
"invitations": "members",
"canBePrivate": true,
"switcherViews": [
{
"enabled": true,
"viewType": "Board"
},
{
"enabled": true,
"viewType": "Table"
},
{
"enabled": false,
"viewType": "Calendar"
},
{
"enabled": false,
"viewType": "Dashboard"
},
{
"enabled": false,
"viewType": "Timeline"
},
{
"enabled": false,
"viewType": "Map"
}
],
"backgroundTile": false,
"backgroundColor": "#DCEAFE",
"backgroundImage": "https://d2k1ftgv7pobq7.cloudfront.net/images/backgrounds/gradients/bubble.svg",
"canBeEnterprise": true,
"permissionLevel": "org",
"sharedSourceUrl": null,
"backgroundTopColor": "#E9F2FE",
"showCompleteStatus": true,
"backgroundDarkColor": "#172F53",
"backgroundDarkImage": "https://d2k1ftgv7pobq7.cloudfront.net/images/backgrounds/gradients/bubble-dark.svg",
"calendarFeedEnabled": false,
"backgroundBrightness": "dark",
"backgroundBottomColor": "#CFE1FD",
"backgroundImageScaled": null,
"hiddenPluginBoardButtons": []
},
"closed": false,
"pinned": false,
"descData": null,
"shortUrl": "https://trello.com/b/NE1R",
"labelNames": {
"red": "Error",
"sky": "project:cascade",
"lime": "",
"pink": "",
"black": "",
"green": "Processed",
"orange": "project:bdgt",
"purple": "Processing",
"yellow": "Auto",
"red_dark": "",
"sky_dark": "",
"blue_dark": "",
"lime_dark": "",
"pink_dark": "",
"red_light": "",
"sky_light": "",
"black_dark": "",
"blue_light": "",
"green_dark": "",
"lime_light": "",
"pink_light": "",
"black_light": "",
"green_light": "",
"orange_dark": "",
"purple_dark": "",
"yellow_dark": "",
"orange_light": "",
"purple_light": "",
"yellow_light": ""
},
"idEnterprise": null,
"idOrganization": "685e770eb2d98a11"
},
"action": {
"id": "69d82b43914131c4a",
"data": {
"old": {
"idList": "69d68913d3c103ebc1"
},
"card": {
"id": "69d82b2306e1456038",
"name": "Add ability to delete uploaded bank statements",
"idList": "69d68925a3d3c103ebc3",
"idShort": 16,
"shortLink": "lm5G4J"
},
"board": {
"id": "69d689125a3d3c103eb7e",
"name": "Cascade",
"shortLink": "NEJV1R"
},
"listAfter": {
"id": "69d6891f7a3d3c103ebc3",
"name": "Planning"
},
"listBefore": {
"id": "69d6891f7325ac103ebc1",
"name": "Backlog"
}
},
"date": "2026-04-09T22:42:11.316Z",
"type": "updateCard",
"limits": null,
"display": {
"entities": {
"card": {
"id": "69d82b2306277c1456038",
"text": "Add ability to delete uploaded bank statements",
"type": "card",
"idList": "69d6891f73a3d3c103ebc3",
"shortLink": "lm554J"
},
"listAfter": {
"id": "69d6891f7323d3c103ebc3",
"text": "Planning",
"type": "list"
},
"listBefore": {
"id": "69d6891f25a3d3c103ebc1",
"text": "Backlog",
"type": "list"
},
"memberCreator": {
}
},
"translationKey": "action_move_card_from_list_to_list"
},
"appCreator": null,
"memberCreator": {
}
},
"webhook": {
"id": "69d768999c617f2a0",
"active": true,
"idModel": "69d6891f7c103eb7e",
"description": "CASCADE webhook",
"consecutiveFailures": 0,
"firstConsecutiveFailDate": null
}
}When multiple projects share the same Trello board and use requiredLabelId to distinguish which cards belong to each project, the previous code only checked the first matching project (via find()), causing bdgt-labeled cards to be dispatched to the cascade project when cascade was first in the list. Add resolveAllProjects() to RouterPlatformAdapter (optional) and implement it in TrelloRouterAdapter to return all projects matching a board ID. Update processRouterWebhook to iterate over all candidate projects and dispatch to the first one whose label check passes, using that matched project for postAck, buildJob, and job enqueue so the correct projectId flows through the entire pipeline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suda
left a comment
There was a problem hiding this comment.
Summary
LGTM — the required-label filter is well-designed and well-tested. One naming inconsistency to fix and two undocumented assumptions worth noting.
Code Issues
Should Fix
- src/router/adapters/trello.ts,
dispatchWithCredentialssignature — The first parameter is named_event(underscore prefix = intentionally unused by convention), but the method now actively reads_event.workItemIdin the label-check block. Comparegithub.ts, which names the same parameterevent(no underscore) when it is consumed. Rename toeventfor consistency and to avoid misleading readers.
Comments (no action required)
-
src/router/adapters/trello.ts
parseWebhook— Usesconfig.projects.find()(first match) to evaluateisCardInTriggerListandisReadyToProcessLabelAdded. This is correct when all shared-board projects use identical list IDs and the samereadyToProcesslabel — the expected configuration for the multi-project use case. It would silently drop events if two projects on the same board were configured with differentreadyToProcesslabel IDs. Worth a brief inline comment to document the assumption. -
src/router/webhook-processor.ts, multi-project dispatch loop — The loop breaks on the first non-null dispatch. A project with no
requiredLabelId(catch-all) placed before filtered projects will always win, making subsequent filtered projects unreachable for matching events. This is correct by design, but operators setting up shared-board projects need to know about this ordering constraint. Worth a note in CLAUDE.md or the wizard UI help text.
suda
left a comment
There was a problem hiding this comment.
Unfortunately the run is still being assigned to the wrong project. Interestingly both projects (cascade and bdgt) have the required label set, therefore it I would assume that the incoming webhook would not be routed at all rather than being routed to the wrong project.
I see that the webhook payload from Trello does not pass the labels assigned to the card, just the information about from which and to which board it was moved. It might be necessary to look up the labels first before routing.
The Trello webhook payload does not include the labels currently assigned to a card — only movement information (listBefore/listAfter). When multiple projects share the same board and are distinguished by requiredLabelId, the previous implementation relied on dispatchWithCredentials (called per-project) to discover a label mismatch. This caused the first project in the list (cascade) to be selected even when the card carried a different project's label. This fix moves the label lookup to resolveAllProjects, which now: - Fetches the card's labels via the Trello API once, before the dispatch loop - Returns only projects whose requiredLabelId matches the card's labels - Falls back to catch-all projects (no requiredLabelId) when no specific match - Returns all candidates if the API call fails (preserving the existing per-project guard in dispatchWithCredentials as a secondary check) Also renames _event → event in dispatchWithCredentials (the parameter is actively read, so the underscore prefix was misleading). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@suda Thanks for the detailed diagnosis! You're right — the Trello webhook payload for The root issue was architectural: Fix applied:
This ensures the correct project is selected before any dispatch attempt. The |
suda
left a comment
There was a problem hiding this comment.
Summary
The feature is well-structured and the multi-project routing logic is clearly reasoned. However, there is a logic flaw in webhook-processor.ts: returning an empty array from resolveAllProjects (label mismatch — "don't process this event") incorrectly falls through to resolveProject, triggering an extra Trello API call and violating the method's contract. In the happy path this also means a double getCard call per filtered event.
Code Issues
Should Fix
src/router/webhook-processor.ts:337-344 — resolveAllProjects([]) incorrectly falls through to resolveProject
When resolveAllProjects is implemented and returns [] (label mismatch), the fallback treats it identically to "method not implemented":
const projectsToTry: RouterProjectConfig[] = [];
if (adapter.resolveAllProjects) {
const allProjects = await adapter.resolveAllProjects(event);
projectsToTry.push(...allProjects); // pushes nothing when []
}
if (projectsToTry.length === 0) { // true even when resolveAllProjects returned []
const singleProject = await adapter.resolveProject(event); // ← incorrectly called
if (singleProject) projectsToTry.push(singleProject);
}Concrete scenario: project has requiredLabelId, resolveAllProjects returns [] because the card lacks the label → falls through to resolveProject which returns the project by boardId → dispatchWithCredentials is invoked → secondary guard calls checkCardHasRequiredLabel (extra Trello API call) → correctly returns null. Outcome is correct but the path is wrong and wastes an API call. It also logs "No trigger matched" instead of the accurate "No project config found".
The correct branching:
let projectsToTry: RouterProjectConfig[] = [];
if (adapter.resolveAllProjects) {
projectsToTry = await adapter.resolveAllProjects(event);
} else {
const singleProject = await adapter.resolveProject(event);
if (singleProject) projectsToTry = [singleProject];
}There is also no test covering this case: resolveAllProjects returns [] → should short-circuit as "no project config found". The existing test "returns No trigger matched when all projects dispatch null" uses a non-empty array from resolveAllProjects and relies on dispatch returning null — a different path.
src/router/adapters/trello.ts:200-213 — double getCard API call in happy path
When resolveAllProjects successfully pre-filters and returns matched projects, dispatchWithCredentials still unconditionally calls checkCardHasRequiredLabel (a second getCard call). In the standard happy path for a project with requiredLabelId:
resolveAllProjects→getCard(call 1) → returns[project]dispatchWithCredentials→checkCardHasRequiredLabel→getCard(call 2)
The secondary guard is documented as a safety net for when resolveAllProjects errored and returned all candidates unfiltered. However it fires unconditionally in the happy path too, doubling Trello API consumption for every webhook event on label-filtered projects. If the fallback logic in webhook-processor.ts is fixed per above (so resolveAllProjects([]) short-circuits), the guard could be conditioned on whether pre-filtering was definitive.
…ble getCard call - Fix webhook-processor.ts: when adapter implements resolveAllProjects, use its result directly (even []) instead of falling through to resolveProject. An empty result means the event was definitively filtered (label mismatch) and must not re-introduce excluded projects. - Eliminate double getCard API call in happy path: mark projects returned by successful resolveAllProjects pre-filtering with _labelPreFiltered, causing dispatchWithCredentials to skip its secondary label guard. The guard still fires when resolveAllProjects fell back to all candidates due to an API error. - Add RouterProjectConfig._labelPreFiltered internal field to router config. - Add test: resolveAllProjects([]) short-circuits as "no project config found" without calling resolveProject or dispatchWithCredentials. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suda
left a comment
There was a problem hiding this comment.
Summary
The feature is well-designed and thoroughly tested, but there is one blocking bug that will silently prevent the label filter from ever activating in production.
Blocking
requiredLabelId stripped by Zod validation in src/config/schema.ts — feature never activates at runtime
ProjectConfigSchema (lines 56–67 of src/config/schema.ts) defines the Zod schema for the Trello config object but does not include requiredLabelId. Zod strips unknown keys by default in .object().parse(), so the field is silently discarded every time config is loaded from the database.
Runtime path:
DB JSONB (has requiredLabelId)
→ mapProjectRow / buildTrelloConfig ✓ field present
→ validateConfig(rawConfig) ✗ Zod strips unknown field
→ ProjectConfig.trello.requiredLabelId === undefined (always)
Both loadAllProjectConfigs (line 81) and findProjectConfigFromDb (line 104) in configRepository.ts call validateConfig, so every router/worker config load hits this path.
Consequence: project.trello?.requiredLabelId is always undefined at runtime. The pre-filter in resolveAllProjects and the secondary guard in dispatchWithCredentials never activate. Every card is processed regardless of its labels.
The unit tests don't catch this because they mock loadProjectConfig directly, bypassing Zod entirely.
Fix — add one line to src/config/schema.ts inside the trello object schema, after the customFields block:
requiredLabelId: z.string().optional(),…pping `ProjectConfigSchema` in `src/config/schema.ts` was missing `requiredLabelId` from the trello object schema. Zod strips unknown keys by default, so the field was silently discarded every time config was loaded from the database via `validateConfig()`. This meant `project.trello?.requiredLabelId` was always `undefined` at runtime, causing the required-label filter to never activate. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suda
left a comment
There was a problem hiding this comment.
Summary
LGTM — well-designed, correctness-first implementation of label-based routing with solid test coverage across all edge cases.
Architecture & Design
The two-layer defense is appropriate:
- **Pre-filter in ** — one API call before the dispatch loop; correct project(s) selected early
- **Secondary guard in ** — fallback when errored and returned all candidates unfiltered
This avoids redundant calls in the happy path while preserving correctness under API failure. The flag is a reasonable internal coupling mechanism, well-documented with JSDoc.
Code Issues
Nitpick
- In (), the
as stringcast oninitialConfig.requiredLabelIdis redundant — the field is already typed asstring | undefinedonTrelloIntegrationConfig. The?? ''handles the undefined case correctly. Harmless. - Catch-all projects in
resolveAllProjectsare marked_labelPreFiltered: truewhen returned from the catch-all branch, even though they have norequiredLabelIdfilter. This is harmless (the secondary guard short-circuits on falsyrequiredLabelIdregardless), but slightly misleading — it implies label verification occurred when it didn't.
Verified
- All CI checks passing (lint, typecheck, unit + integration tests)
- The
resolveAllProjectsloop correctly uses the first project's credentials to make a singlegetCardcall, then routes all candidates based on those labels — correct since all projects sharing a board ID access the same card data - The 👀 reaction intentionally fires before label filtering (step 5 vs step 6); acknowledged in PR description as per spec
- No-
workItemIdedge case: both the pre-filter and secondary guard skip label checks whenworkItemIdis absent, so a card without a known ID falls through to all candidates — acceptable for events where card ID cannot be determined - Axios upgrade (
>=1.15.0override) is a routine security patch, correct approach
suda
left a comment
There was a problem hiding this comment.
I see now that in the Webhook Log Detail the hook has been assigned to the correct project ID bdgt. However I see the agent failing with:
❌ Error: Error: Integration credential 'scm/implementer_token' not found for project 'cascade'
So somewhere next in the flow the wrong project is being picked up.
… resolution When multiple projects share the same Trello board (distinguished by requiredLabelId), the router correctly routes to the right project (e.g. bdgt) and stores projectId in the job. However the worker was re-resolving the project by boardId in processPMWebhook, returning the first matching project (e.g. cascade) instead of the intended one. Fix: pass jobData.projectId from the worker through processTrelloWebhook and processJiraWebhook to processPMWebhook, where it bypasses the boardId-based lookupProject and uses loadProjectConfigById directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
suda
left a comment
There was a problem hiding this comment.
Summary
LGTM — the implementation is correct and the defensive two-layer filtering design is sound.
Observations (non-blocking)
on the catch-all path is redundant but harmless
When falls back to catch-all projects (those without a ), it marks them . The secondary guard in checks first, so the flag is never consulted for catch-all projects (they have no label filter). Zero correctness impact, but it slightly overstates what the flag means.
Events without bypass label filtering entirely
If event.workItemId is absent, resolveAllProjects returns all candidates unfiltered, and the secondary guard in dispatchWithCredentials also skips (condition requires event.workItemId). Both projects on a shared board would have triggerRegistry.dispatch called. In practice this shouldn't matter since every processable card event carries a card ID, but it's an edge case worth noting.
Missing test for skipping the secondary guard
The test suite covers dispatching with and without requiredLabelId, and it covers resolveAllProjects pre-filtering, but no test verifies that a project with _labelPreFiltered: true actually skips the checkCardHasRequiredLabel call in dispatchWithCredentials. The optimization works correctly from code inspection, but a test would lock in the behaviour.
👀 reaction fires before label filtering for comment events
Step 5 fires the reaction, label filtering happens at step 7. A comment on a card lacking the required label will receive a 👀 but no agent action. The PR notes this is per-spec, and the reaction is only sent for comment events while label filtering primarily targets card-move/label-add events — so the practical overlap is narrow.
None of these are blocking. All CI checks pass, tests are thorough (9 new test cases for the core filtering logic, 4 for the adapter dispatch paths), and the end-to-end worker path correctly threads projectId through to the PM webhook processor.
Summary
requiredLabelIdconfig field to Trello integration that restricts webhook processing to cards carrying a specific Trello labelTrelloRouterAdapter.dispatchWithCredentials(within credential scope, before ack comments are posted)TrelloFieldMappingStepwizard UIDetails
Config layer
requiredLabelId?: stringtoTrelloConfig(src/pm/config.ts),TrelloIntegrationConfig(src/db/repositories/configMapper.ts), andProjectConfigRaw['trello']requiredLabelIdthroughbuildTrelloConfigandRouterProjectConfig.trelloBackend filtering
checkCardHasRequiredLabel(cardId, requiredLabelId)helper inrouter/trello.ts: returnstruewhen no filter is set, or checks card labels viatrelloClient.getCard()API callwithTrelloCredentialscallback indispatchWithCredentials— only fires whenrequiredLabelIdis configured ANDworkItemIdis known. If card lacks the label, logs atinfolevel and returnsnullFrontend wizard
trelloRequiredLabelIdfield toWizardState,WizardAction, initial state, board reset, and edit state restore (buildEditState)SET_TRELLO_REQUIRED_LABELreducer caseTrelloFieldMappingStepwith help text, below the Cost field, following the same searchable dropdown + manual fallback patternrequiredLabelIdinuseSaveMutationconfig (only when non-empty)Unit tests
tests/unit/router/trello.test.ts: 5 new tests forcheckCardHasRequiredLabeltests/unit/router/adapters/trello.test.ts: 4 new tests for label filtering indispatchWithCredentialsgitlabOnly, credential-scoping test not clearingGITHUB_TOKEN_IMPLEMENTER/GITLAB_TOKEN_IMPLEMENTER)Notes
requiredLabelIdis configured (opt-in, zero cost otherwise)dispatchWithCredentials, so filtered cards still get the emoji (per spec)Trello card: https://trello.com/c/iELcotDw/11-in-the-trello-integration-configuration-under-field-mappings-custom-field-add-an-option-for-required-label-that-can-be-left-blan
🤖 Generated with Claude Code