fix(jira): match status by locale-invariant ID with name fallback (MNG-1768) - #1528
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Correctly implements MNG-1768 — JIRA status matching is now locale-invariant (status ID first, case-insensitive name fallback) on both the dispatch (resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions) and move (moveWorkItem → to.id) sides, plus a self-healing wizard migration (normalizeJiraStatusMappingsToIds on SET_JIRA_PROJECT_DETAILS). The change matches all 7 implementation steps in the work item, keeps name-based configs working (verified via retained back-compat tests and the built-in todo → implementation resolution), and CI is green (7/7). No blocking issues — a few considerations below.
Code Issues
Should Fix
src/pm/jira/adapter.ts—moveWorkItemSentry capture on benign no-ops. The newcaptureException({ jira_transition_not_found })fires on any miss, butmoveWorkItemis called best-effort fromcreateWorkItem(thebacklogtransition,adapter.ts:214) and from lifecyclesafeMove(moveOnPrepare/moveOnSuccess,src/pm/lifecycle.ts:187). Neither checks whether the issue is already indestination. A freshly-created issue that JIRA already placed in the configuredbacklogstatus has no self-transition, so this will emit a Sentry event for a benign no-op — potentially on every issue creation — diluting the exact genuine locale/misconfig signal the ticket wants to make loud. Consider skipping the capture when the current status already equalsdestination(or when invoked from the best-effort create path).
Documentation
src/integrations/README.md:298anddocs/architecture/04-agent-system.md:144still state the JIRA trigger resolves viaresolvePMStatusAgentByNameFromWorkflowDefinitions. This PR replaced that call withresolvePMStatusAgentByIdOrNameFromWorkflowDefinitions, soREADME.mdnow contradicts its own newly-added subsection (~L273). Worth updating both for consistency given the repo's docs-drift discipline (04-agent-system.mdis outside this diff).
Nitpicks (optional)
src/pm/jira/adapter.ts— the legacyt.id === destinationfallback (transition ID) can now collide with a numeric status-IDdestination(e.g. a small system status id equal to some transition'sid). Theto.id-first ordering makes this very unlikely, but the branch appears vestigial (no caller passes a transition ID) — consider dropping it.tests/unit/triggers/shared/pm-status.test.ts— the "prefers the ID match when both id and name are supplied" case passes becauseplanningis the first object entry (and matches by id), not because of a global id-over-name preference; reversing entry order would let the name match ontodowin. The test is valid, but the comment overstates the guarantee.
🕵️ claude-code · claude-opus-4-8 · run details
| // account whose transitions never match `destination` is otherwise | ||
| // invisible — this capture surfaces it on the first run. No-op when | ||
| // SENTRY_DSN is unset. | ||
| captureException(new Error('No JIRA transition found for destination'), { |
There was a problem hiding this comment.
moveWorkItem unconditionally captures jira_transition_not_found on a miss, but it is called best-effort from createWorkItem (the backlog transition at adapter.ts:214) and from lifecycle safeMove (moveOnPrepare / moveOnSuccess). Neither checks whether the issue is already in destination. A newly-created issue that JIRA already placed in the configured backlog status has no self-transition, so this fires a Sentry capture for a benign no-op — potentially on every issue creation — diluting the genuine locale/misconfig signal this tag is meant to surface. Consider skipping the capture when the current status already equals destination (or when called from the best-effort create path).
There was a problem hiding this comment.
Fixed in c40449d. moveWorkItem now checks whether the issue is already in destination before capturing on the miss path, so the jira_transition_not_found signal stays meaningful.
Rather than plumb a "best-effort" flag through createWorkItem and lifecycle safeMove (moveOnPrepare/moveOnSuccess) separately, I added a small isAlreadyInStatus(id, destination) helper that fetches the current status and matches it locale-invariantly (status ID first, case-insensitive name fallback — mirroring the transition matcher). When the issue is already there, we logger.debug a no-op and return without capturing; any read failure returns false so a genuine miss is never suppressed. This covers all best-effort callers uniformly (the freshly-created-issue-already-in-backlog case included) at the cost of one extra getIssue only on the already-rare miss path.
While here, I also dropped the vestigial t.id === destination (transition-ID) fallback branch you flagged in the nitpick — no caller passes a transition ID and it could collide with a numeric status-ID destination.
New coverage in tests/unit/pm/jira/adapter.test.ts: benign no-op by ID, benign no-op by name (back-compat), and an explicit-current-status genuine-miss case that still captures.
Address review feedback on PR #1528 (MNG-1768): - moveWorkItem no longer fires the jira_transition_not_found Sentry capture when the issue is already in the destination status. Best-effort callers (createWorkItem's backlog move, lifecycle moveOnPrepare/ moveOnSuccess) move unconditionally and JIRA offers no self-transition, so an already-there issue legitimately reaches the miss path — capturing there diluted the genuine locale/misconfig signal. Added isAlreadyInStatus helper (ID-first, name fallback) so only a real miss is surfaced loudly. - Dropped the vestigial `t.id === destination` (transition-ID) matcher branch that could collide with a numeric status-ID destination; no caller passes a transition ID. - Docs: src/integrations/README.md and docs/architecture/04-agent-system.md now reference resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions for the JIRA status-changed trigger. - Clarified the overstated pm-status "prefers the ID match" test comment (per-entry ID-before-name, first matching entry wins during iteration). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Requesting changes. The core MNG-1768 fix — locale-invariant ID-or-name matching for JIRA status-changed dispatch (resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions) and moveWorkItem (to.id first), plus the self-healing wizard migration and the jira_transition_not_found Sentry signal — is correct, cleanly implemented, and well-tested. But the PR changes the semantics of every jira.statuses config value from name → ID and only updates two of the four consumers of that map. Two other JIRA triggers still match the issue's status.name by name only and will silently stop firing once a project's config holds IDs.
Architecture & Design
-
[BLOCKING] Incomplete read-side migration of
jira.statuses(name → ID). The write side now persists status IDs (jira/wizard.ts:510,providerStates → { id: s.id }) and auto-migrates legacy name configs to IDs (jira/state.tsnormalizeJiraStatusMappingsToIdsonSET_JIRA_PROJECT_DETAILS).status-changeddispatch andmoveWorkItemwere updated to match ID-or-name — but two other readers of the samejira.statusesmap were not:src/triggers/jira/label-added.ts—JiraReadyToProcessLabelTriggerreadspayload.issue.fields.status.name(L77) and resolves the agent viaresolvePMLabelAgentByStatusNameFromWorkflowDefinitions({ statusName, configuredStatuses: jiraConfig.statuses })(L91–94), which matches by name only (caseInsensitiveStatusMatcher).src/triggers/jira/comment-mention.ts—isInPlanningStatuscomparespayload.issue.fields.status.name(L181) againstjiraConfig.statuses.planningby name (L89).planningis one of the wizard'sJIRA_STATUS_SLOTS, so it is migrated to an ID.
Failing scenario (new JIRA project, fully silent): operator maps
planning → "Planning"in the updated wizard → config persistsstatuses.planning = "10005"; enablespm:comment-mentionforrespond-to-planning-comment. A user @mentions the bot on an issue currently in Planning.isInPlanningStatuscomparesstatus.name"Planning"againststatuses.planning"10005"→ no match →handle()returnsnullat adebuglog → the agent never runs. Thecascade-readylabel flow breaks identically ("To Do"vs"10010"). This is the exact silent-no-op class MNG-1768 set out to eliminate, reintroduced on a different surface. Existing JIRA projects are safe only until their config is re-saved; new projects are affected immediately.Why CI is green: the existing
jira-label-added/jira-comment-mentionsuites use name-basedstatuses({ planning: 'Planning', todo: 'To Do', ... }matched againststatus: { name: 'Planning' }), so name-matching still passes. The ID-config path for these two triggers is untested.Fix is small and already scaffolded here:
types.tsalready widenedissue.fields.statusto{ id?, name? }, sostatus.idis available on both payloads. Add aresolvePMLabelAgentByStatusIdOrNameFromWorkflowDefinitions(mirroring the new shared resolver), makeisInPlanningStatuscompare id-or-name, and passstatus.idalongsidestatus.namefrom both triggers — plus one ID-config test each.If these two flows are intentionally out of scope, that should be stated explicitly: the work item and
README.md/CLAUDE.mdclaim "both ends now match on the ID," which is currently untrue for label-added / comment-mention.
Notes (non-blocking)
moveWorkItemID-or-name matching + dropping the vestigialt.id === destinationbranch is correct. TheisAlreadyInStatusguard against spuriousjira_transition_not_foundcaptures is a good call, and a read failure correctly falls through so genuine misses still surface.- Docs (
CLAUDE.md/AGENTS.mdsymlink,README.md,04-agent-system.md) are consistent with the status-changed / move changes.
🕵️ claude-code · claude-opus-4-8 · run details
| // (`s.id`), while the human-readable `s.name` is what the select | ||
| // displays. Previously both were `s.name`, which made status moves | ||
| // locale-fragile. | ||
| providerStates: (details?.statuses ?? []).map((s) => ({ id: s.id, name: s.name })), |
There was a problem hiding this comment.
MNG-1768 write side: persisting status IDs here (and auto-migrating legacy configs via normalizeJiraStatusMappingsToIds) changes the semantics of every jira.statuses value. But only status-changed dispatch and moveWorkItem were updated to match by ID — two other consumers of the same map still match the issue's status.name by name only and will silently stop firing for ID-based configs (all new projects, plus any re-saved project):
src/triggers/jira/label-added.ts(L77, L91-94) —JiraReadyToProcessLabelTrigger, thecascade-readyflow.src/triggers/jira/comment-mention.ts(L81, L181) —isInPlanningStatus, gatingrespond-to-planning-comment;planningis a migratedJIRA_STATUS_SLOTSentry.
Both need to read status.id (already typed via this PR's types.ts widening) and match via an id-or-name resolver, mirroring resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions. See the review summary for the exact failing scenario.
There was a problem hiding this comment.
Fixed in 4cf0ca2. Migrated both remaining jira.statuses readers to id-or-name matching, mirroring resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions:
- New shared resolver —
resolvePMLabelAgentByStatusIdOrNameFromWorkflowDefinitionsinsrc/triggers/shared/pm-label.ts, delegating to the status resolver so custom-status + null-agentTypehandling stays identical. label-added.ts(JiraReadyToProcessLabelTrigger, thecascade-readyflow) — now reads bothstatus.idandstatus.nameand resolves via the new id-or-name resolver.comment-mention.ts—isInPlanningStatusnow compares the configuredplanningvalue against the issue'sstatus.idfirst, then case-insensitively againststatus.name; the caller passes both.
No collision risk between the branches: JIRA status IDs are numeric strings while names are free text, so a configured value matches at most one interpretation.
Added ID-config unit tests for each trigger (foreign-language name + matching ID, e.g. "En cours" / 10010) plus three cases for the new shared resolver, and documented both readers in the MNG-1768 README subsection so the "every consumer matches by ID" claim is now accurate. npm run typecheck, biome, and the full unit-triggers suite (1284 tests) pass.
…ion (MNG-1768) The MNG-1768 write side persists locale-invariant JIRA status IDs into `jira.statuses`, but only status-changed dispatch and moveWorkItem were migrated to match by ID. Two other readers of the same map still matched the issue's `status.name` by name only, so they silently stopped firing for ID-based configs (all new projects, plus any re-saved project): - `JiraReadyToProcessLabelTrigger` (cascade-ready label flow) - `JiraCommentMentionTrigger.isInPlanningStatus` (planning gate) Add `resolvePMLabelAgentByStatusIdOrNameFromWorkflowDefinitions` (mirroring the shared status resolver), read `status.id` alongside `status.name` in both triggers, and gate planning by ID-first with a case-insensitive name fallback. Now every consumer of the ID-valued `jira.statuses` map is locale-invariant. Adds ID-config unit tests for each trigger and the new resolver, and documents the two readers in the MNG-1768 README subsection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
nhopeatall
left a comment
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
APPROVE — MNG-1768 is implemented correctly and, importantly, completely. JIRA status matching is now locale-invariant (status ID first, case-insensitive name fallback) on every surface that reads jira.statuses, existing name-based configs keep working, and the previously-silent transition miss is now a Sentry signal. CI is green (7/7).
I traced every consumer of jira.statuses to confirm the read-side migration has no gaps (the class of bug flagged in the prior review round):
- Live-status matchers (the bug class) — all migrated to id-or-name:
src/triggers/jira/status-changed.ts→resolvePMStatusAgentByIdOrNameFromWorkflowDefinitionssrc/triggers/jira/label-added.ts→resolvePMLabelAgentByStatusIdOrNameFromWorkflowDefinitionssrc/triggers/jira/comment-mention.ts→isInPlanningStatus(ID first, name fallback)
- Destinations passed to
moveWorkItem(which now matchesto.idfirst): lifecyclemoveOnPrepare/moveOnSuccess,pr-ready-to-merge(merged/done),createWorkItembacklog move — all covered by the same id-or-name matcher. - Presence checks / JQL filter (unaffected by name→ID):
backlog-check,contextSteps(presence check only),listWorkItems(JQL — see below).
moveWorkItem's to.id-first matcher, the isAlreadyInStatus benign-no-op guard (correctly returning false on read failure so a genuine miss is never suppressed), and the wizard's self-healing normalizeJiraStatusMappingsToIds (id-set guard first, name→id rewrite, unknown/custom untouched, same-reference when unchanged) are all sound and well-covered by the new tests.
Questions / Considerations (non-blocking)
listWorkItemsJQL relies on JIRA resolving a quoted numeric status ID (status = "<id>",src/pm/jira/adapter.ts). This is the one correctness point that depends on an external JIRA contract the code can't self-verify, and it feeds the backlog-manager pipeline snapshot (contextSteps→listWorkItems({ status: statusKey }), which now resolves to an ID).fetchPipelineListswraps each call in try/catch so a failure degrades per-list rather than crashing — but it would degrade silently to an empty list for ID-based JIRA configs if the quoted-ID assumption is wrong. The work item documents this as a considered decision; just flagging that the described manual test exercised the transition/move path, not this JQL read path — worth a one-time confirmation against a live JIRA instance with an ID-based config.
Nitpicks (optional)
- After this PR,
resolvePMStatusAgentByNameFromWorkflowDefinitionsandresolvePMLabelAgentByStatusNameFromWorkflowDefinitionsare no longer used by any production trigger (only by each other and their tests) — JIRA was the last caller and it moved to the id-or-name variants. Candidates for removal in a follow-up unless retained deliberately as shared toolkit.
🕵️ claude-code · claude-opus-4-8 · run details
| // | ||
| // MNG-1768: config.statuses values are now status IDs (locale-proof), | ||
| // with names accepted as a legacy fallback. JQL accepts a quoted | ||
| // status ID (`status = "10010"`) just as it accepts a quoted name, so |
There was a problem hiding this comment.
Non-blocking: this JQL path now emits status = "10010" (quoted numeric ID) for migrated configs, and it backs the backlog-manager pipeline snapshot via contextSteps → listWorkItems({ status: statusKey }). Correctness here depends on JIRA resolving a quoted numeric value against status IDs — an external contract the code can't self-verify. fetchPipelineLists try/catches per list, so a wrong assumption degrades silently to an empty list rather than erroring. Worth a one-time confirmation against a live JIRA with an ID-based config, since the described manual test exercised the transition/move path (moveWorkItem), not this read path.
2 participants
Summary
Fixes MNG-1768 — JIRA status matching was name-based on both ends, so status moves silently no-op'd when the credential account's language differed from the webhook (site) language. This PR makes matching locale-invariant by keying on the JIRA status ID, with name matching kept as a fallback (zero forced migration), and makes the silent miss loud via Sentry.
🔗 https://linear.app/issue/MNG-1768
What changed
Locale-invariant ID matching on both ends
src/triggers/shared/pm-status.ts— newresolvePMStatusAgentByIdOrNameFromWorkflowDefinitions({ statusId, statusName, configuredStatuses }). Reuses the existing workflow-definition iterator via a closure matcher: matches when the stored config value exactly equalsstatusIdor case-insensitively equalsstatusName(ID checked first). Custom workflow statuses and the null-agentTypeguard keep working unchanged.src/triggers/jira/types.ts— widened the webhook payload types:issue.fields.status.id?, andchangelog.items[].from?/.to?(the status IDs) alongside the existingfromString/toStringnames.src/triggers/jira/status-changed.ts—resolveNewStatusnow returns{ id?, name? }(create path →issue.fields.status.{id,name}; update path → changelog{to, toString}).handle()resolves via the new id-or-name resolver;matches()accepts an id or name on the create path. LogstoStatusIdnext totoStatus.src/pm/jira/adapter.ts—moveWorkItemmatcher now preferstransitions[].to.id === destination(the target status ID, distinct from the transitiont.id), keeping the name/t.idbranches as fallbacks.JiraTransition.towidened to{ id?, name? }. A genuine no-transition-found miss now emits a SentrycaptureExceptiontaggedjira_transition_not_foundin addition to the existing WARN. Documented that JQLlistWorkItemsaccepts a quoted status ID unchanged.Self-healing wizard migration
web/.../jira/wizard.ts— the status-mapping select now persists{ id: s.id, name: s.name }(status ID as the value, name displayed) instead of{ id: s.name, name: s.name }.web/.../jira/state.ts— newnormalizeJiraStatusMappingsToIds(mappings, statuses), invoked in theSET_JIRA_PROJECT_DETAILSreducer. When project details load, legacy name-valued mappings auto-upgrade to IDs (case-insensitive); already-ID and unrecognized/custom values are left untouched. Re-saving any project backfills IDs.Docs
src/integrations/README.md— JIRA row in the custom-workflow-status table updated to note IDs (name as legacy fallback); new "JIRA status matching is ID-based" subsection.CLAUDE.md/AGENTS.md(symlinked, stay byte-identical) — new "JIRA status matching (locale-invariant)" section.Testing
resolvePMStatusAgentByIdOrNameFromWorkflowDefinitions: id-match / name-match / id-preferred / no-match / custom-status / null-agent.toStringdispatches), name-based back-compat, create path viastatus.id,toStatusIdlogging.moveWorkItemmatches byto.id(foreign-languageto.name), Sentryjira_transition_not_foundcapture on a genuine miss, name-based back-compat retained (no capture).npm run typecheck, biome lint on changed files, PM conformance harness, and the docs-drift guard all pass.🤖 Generated with Claude Code
🕵️ claude-code · claude-opus-4-8 · run details