Skip to content

fix(pm): consolidate PM registration to single canonical path in integrations/bootstrap.ts#1078

Merged
aaight merged 2 commits intodevfrom
fix/dual-pm-registration-path
Apr 3, 2026
Merged

fix(pm): consolidate PM registration to single canonical path in integrations/bootstrap.ts#1078
aaight merged 2 commits intodevfrom
fix/dual-pm-registration-path

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Apr 3, 2026

Summary

  • Remove side-effect registration from src/pm/index.ts (lines 30-44) — eliminates the redundant registration of Trello/Jira integrations at import time
  • Delete src/pm/bootstrap.ts — dead file (never imported in production, only mocked in one test)
  • Update JSDoc comments in src/pm/registry.ts and src/integrations/registry.ts to accurately reflect the single registration path
  • Update CLAUDE.md documentation to describe the canonical registration point
  • Fix affected tests to import src/integrations/bootstrap.js instead of relying on src/pm/index.js side effects

Details

There were three registration paths for PM integrations:

  1. src/pm/index.ts — side-effect registration at import time (removed)
  2. src/pm/bootstrap.ts — dead duplicate file (deleted)
  3. src/integrations/bootstrap.ts — the canonical registration path (kept, now the only one)

Both router and worker already import src/integrations/bootstrap.ts at the top level, so the idempotent guards already made the side-effect registrations in src/pm/index.ts into no-ops at runtime. This change makes the code reflect that reality.

The following test files were updated to import src/integrations/bootstrap.js (with necessary mocks) instead of relying on the now-removed side effects from src/pm/index.js:

  • tests/unit/pm/factory.test.ts
  • tests/unit/pm/lifecycle.test.ts
  • tests/unit/cli/credential-scoping.test.ts
  • tests/unit/triggers/jira-label-added.test.ts
  • tests/unit/triggers/pr-merged.test.ts
  • tests/unit/triggers/pr-ready-to-merge.test.ts
  • tests/unit/router/webhook-signature.test.ts (removed dead mock for deleted file)

Test plan

  • All 7241 unit tests pass (npm run test:unit)
  • TypeScript type checking passes (npm run typecheck)
  • Lint passes with zero errors (npm run lint)

Card: https://trello.com/c/69ce1c1d12d7f6885bbd9ca0

🤖 Generated with Claude Code

🕵️ claude-code · claude-sonnet-4-6 · run details

Cascade Bot and others added 2 commits April 3, 2026 16:05
…test

After removing the side-effect registration from src/pm/index.ts, the
pm-provider-switching integration test no longer had pmRegistry populated,
causing the pmRegistry assertions and createPMProvider calls to fail.
Add the canonical bootstrap import (matching integration-validation.test.ts).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@aaight
Copy link
Copy Markdown
Collaborator Author

aaight commented Apr 3, 2026

CI Failures Resolved

Fixes Applied

  • Added bootstrap import to integration test: tests/integration/pm-provider-switching.test.ts was missing import '../../src/integrations/bootstrap.js' — after the PR removed side-effect registration from src/pm/index.ts, the pmRegistry and createPMProvider calls in this test had an empty registry, causing integration test failures

Root Cause

The pm-provider-switching.test.ts integration test relied on the removed side-effect registration from src/pm/index.ts (previously, importing createPMProvider would automatically register Trello/Jira integrations). The PR correctly removed those side effects, but this integration test wasn't updated alongside the 7 unit tests that were fixed.

The fix mirrors the pattern already used in integration-validation.test.ts, which correctly imports src/integrations/bootstrap.js to populate the registry before running tests.

Verification

  • TypeScript type checking passes (zero errors)
  • Lint passes (zero errors)
  • Changes pushed to branch

🕵️ claude-code · claude-sonnet-4-6 · run details

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Clean consolidation of PM integration registration to a single canonical path. The change correctly removes redundant registration code that was already a no-op at runtime, and the documentation updates accurately reflect the new state. All CI checks pass. LGTM.

Should Fix (non-blocking)

Stale JSDoc on IntegrationRegistry class (src/integrations/registry.ts:1-9)

The class-level JSDoc still says "Populated at import time by each integration module" — this contradicts the updated singleton comment on line 77 ("populated at bootstrap time by src/integrations/bootstrap.ts"). Only the singleton export comment was updated; the class-level comment was missed.

4 test files still import src/pm/index.js for (now-dead) side-effect registration

These files were not updated in this PR and still have import '../../../src/pm/index.js' with a comment "Register PM integrations in the registry":

  • tests/unit/triggers/label-added.test.ts:25
  • tests/unit/triggers/status-changed.test.ts:26
  • tests/unit/triggers/merged-status-changed.test.ts:32
  • tests/unit/triggers/backlog-status-changed.test.ts:26

Since registration side effects were removed from src/pm/index.ts, these imports are now no-ops. The tests still pass because they don't actually need the pmRegistry to be populated (they mock the clients directly and test trigger matching logic). However, having some tests import integrations/bootstrap.js and others still import the dead pm/index.js for "registration" is inconsistent — a future developer might copy the wrong pattern.

Not blocking because tests pass and it's a consistency issue, not a correctness one.

🕵️ claude-code · claude-opus-4-6 · run details

@aaight aaight merged commit 12dd917 into dev Apr 3, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants