From 75ee6de6056ea26a45a9d25c17b177516235f8e6 Mon Sep 17 00:00:00 2001 From: zbigniew sobiecki Date: Wed, 15 Apr 2026 21:57:54 +0200 Subject: [PATCH 1/4] docs(plans): add spec 004 + plan; lock plan 004/1 Spec 004 targets the credential role-resolution ambiguity that breaks Linear wizard reopens and silently skips Linear webhook signature verification in the router. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../1-disambiguate-role-resolver.md.wip | 321 ++++++++++++++++++ .../_coverage.md | 37 ++ ...credential-role-provider-disambiguation.md | 153 +++++++++ 3 files changed, 511 insertions(+) create mode 100644 docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip create mode 100644 docs/plans/004-credential-role-provider-disambiguation/_coverage.md create mode 100644 docs/specs/004-credential-role-provider-disambiguation.md diff --git a/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip new file mode 100644 index 00000000..674c56d0 --- /dev/null +++ b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip @@ -0,0 +1,321 @@ +--- +id: 004 +slug: credential-role-provider-disambiguation +plan: 1 +plan_slug: disambiguate-role-resolver +level: plan +parent_spec: docs/specs/004-credential-role-provider-disambiguation.md +depends_on: [] +status: pending +--- + +# 004/1: Disambiguate Credential Role Resolution by Provider + +> Part 1 of 1 in the 004-credential-role-provider-disambiguation plan. See [parent spec](../../specs/004-credential-role-provider-disambiguation.md). + +## Summary + +Single coherent plan closing spec 004. The root cause is one helper — `roleToEnvVarKey(category, role)` in `src/config/provider.ts` — returning the first provider in its registered Map whose role name matches. Because the built-in Map orders PM providers as `trello → jira → linear`, calls for `('pm', 'api_key')` silently resolve to `TRELLO_API_KEY` (breaks Linear-only projects) and `('pm', 'webhook_secret')` silently resolve to `JIRA_WEBHOOK_SECRET` (silently skips Linear webhook signature verification in production). + +The fix widens the helper's signature to `roleToEnvVarKey(category, provider, role)`; propagates the provider through `getIntegrationCredential` and `getIntegrationCredentialOrNull` as a required argument; updates every call site to pass the literal provider (for provider-aware callers) or a freshly-looked-up provider from `project_integrations` (for the dashboard `*ByProject` discovery endpoints and the router's `resolveWebhookSecret`). Type-checked end to end — a missing provider is a build error. + +New error shape for the `*ByProject` endpoints: when the project has no `project_integrations` row for the relevant category, the endpoint throws `NOT_FOUND` with a distinguishable message (`'No PM integration configured for this project yet'` or equivalent), not conflated with `'{provider} credentials not configured'`. + +**Components delivered:** +- `src/config/provider.ts` — widen `roleToEnvVarKey`, `getIntegrationCredential`, `getIntegrationCredentialOrNull`. +- `src/pm/linear/integration.ts`, `src/pm/jira/integration.ts`, `src/pm/trello/integration.ts` — pass `'linear'` / `'jira'` / `'trello'` at every call site. +- `src/github/personas.ts`, `src/github/scm-integration.ts` — pass `'github'`. +- `src/router/github-token-resolver.ts`, `src/router/pre-actions.ts` — pass `'github'`. +- `src/router/reactions.ts` — pass provider (determined from project PM integration). +- `src/router/platformClients/credentials.ts` — pass explicit provider in each branch of `resolveWebhookSecret` and in `resolveTrelloCredentials` / `resolveJiraCredentials` / `resolveLinearCredentials`. +- `src/api/routers/integrationsDiscovery.ts` — each `*ByProject` endpoint looks up the project's provider for the category and passes it down; on missing row, throws the new distinguishable `NOT_FOUND`. +- `tests/unit/config/role-resolver.test.ts` (new) — hermetic unit tests proving disambiguation for `api_key` and `webhook_secret` across all affected provider pairs. +- `tests/integration/api/integrationsDiscovery.test.ts` or equivalent (augment existing if present) — end-to-end assertion that the `*ByProject` endpoints return the right credential on a Linear-only project and throw the new distinguishable error on a project with no integration row. +- `tests/unit/router/resolveWebhookSecret.test.ts` (new or augmented) — asserts `resolveWebhookSecret('linear')` returns the stored `LINEAR_WEBHOOK_SECRET`, not `JIRA_WEBHOOK_SECRET`. + +**Deferred to later plans in this spec:** +- None. Plan 1 closes spec 004. + +--- + +## Spec ACs satisfied by this plan + +- Spec AC #1 (Linear wizard reopens cleanly) — **full** +- Spec AC #2 (Linear wizard survives page refresh) — **full** +- Spec AC #3 (Trello / JIRA wizards enjoy same fix) — **full** +- Spec AC #4 (Shared-role disambiguation including `webhook_secret`) — **full** +- Spec AC #5 (Distinguishable "no integration" error) — **full** +- Spec AC #6 (Helper signature forces disambiguation) — **full** +- Spec AC #7 (Runtime credential path corrects, never regresses) — **full** +- Spec AC #8 (`getOrgCredential` path unchanged) — **full** +- Spec AC #9 (Lint + typecheck + tests green) — **full** +- Spec AC #10 (Zero operator action required post-deploy) — **full** + +--- + +## Depends On + +- Nothing external. Single-plan spec. + +Context to lift from the spec (do not re-argue): +- Fix at the root, not the endpoint. One helper change + all callers updated in one PR. +- `*ByProject` endpoints look up `project_integrations.provider` inline (no new helper). +- `webhook_secret` disambiguation ships in the same change (was a live production bug for Linear webhook signature verification). +- Out of scope: Linear project selection (deferred to a future spec). + +--- + +## Detailed Task List (TDD) + +### 1. Unit tests for the role resolver + +**Tests first** (`tests/unit/config/role-resolver.test.ts` — new file): + +- `roleToEnvVarKey returns Trello's env-var key when provider='trello' and role='api_key'` — assert `'TRELLO_API_KEY'`. +- `roleToEnvVarKey returns Linear's env-var key when provider='linear' and role='api_key'` — assert `'LINEAR_API_KEY'`. This test would fail under the pre-fix helper when order happens to put Trello first (which it does). +- `roleToEnvVarKey returns JIRA's env-var key when provider='jira' and role='webhook_secret'` — assert `'JIRA_WEBHOOK_SECRET'`. +- `roleToEnvVarKey returns Linear's env-var key when provider='linear' and role='webhook_secret'` — assert `'LINEAR_WEBHOOK_SECRET'`. Key test; would fail pre-fix. +- `roleToEnvVarKey returns GitHub's env-var key when provider='github' and role='webhook_secret'` — assert `'GITHUB_WEBHOOK_SECRET'`. +- `roleToEnvVarKey returns Sentry's env-var key when provider='sentry' and role='webhook_secret'` — assert `'SENTRY_WEBHOOK_SECRET'`. +- `roleToEnvVarKey returns undefined when the (provider, role) pair is not registered` — e.g. `('jira', 'api_key')` (JIRA has no `api_key` role). +- `roleToEnvVarKey returns undefined when the provider is registered but the category mismatches` — e.g. `('pm', 'github', 'implementer_token')` — GitHub is SCM, so the mismatch returns undefined even though the role name exists. + +**Implementation** (`src/config/provider.ts`): + +Change the helper signature: +```ts +function roleToEnvVarKey( + category: string, + provider: string, + role: string, +): string | undefined { + const providerRoles = PROVIDER_CREDENTIAL_ROLES[provider as IntegrationProvider]; + if (!providerRoles) return undefined; + const providerCategory = PROVIDER_CATEGORY[provider as keyof typeof PROVIDER_CATEGORY]; + if (!providerCategory || providerCategory !== category) return undefined; + return providerRoles.find((r) => r.role === role)?.envVarKey; +} +``` + +No loop through other providers — the helper indexes directly on the provided provider. + +### 2. Widen `getIntegrationCredential` / `getIntegrationCredentialOrNull` + +**Tests first** (augment `tests/unit/config/role-resolver.test.ts` or new `tests/unit/config/provider.test.ts`): + +- `getIntegrationCredentialOrNull returns the stored credential for the correct (projectId, category, provider, role) tuple` — mock `getResolver().resolve` to return a recognizable value when asked for `'LINEAR_API_KEY'`; call `getIntegrationCredentialOrNull('proj', 'pm', 'linear', 'api_key')`; assert it returns that value. +- `getIntegrationCredentialOrNull returns null when role is not registered for the provider` — e.g. `('proj', 'pm', 'jira', 'api_key')`. +- `getIntegrationCredential throws when credential is missing` — verify the thrown Error message names the provider and role for diagnosability. + +**Implementation** (`src/config/provider.ts`): + +Change both function signatures: +```ts +export async function getIntegrationCredential( + projectId: string, + category: string, + provider: string, + role: string, +): Promise { ... } + +export async function getIntegrationCredentialOrNull( + projectId: string, + category: string, + provider: string, + role: string, +): Promise { ... } +``` + +Both call the new `roleToEnvVarKey(category, provider, role)`. Throw / return null paths unchanged in behavior aside from disambiguation. Thrown error message includes `provider` for diagnosability (currently only includes `category/role`). + +### 3. Update provider-aware call sites + +**Stale-reference check:** grep for `getIntegrationCredential\(` and `getIntegrationCredentialOrNull\(` under `src/`. Confirm the full list matches the counted ~30 call sites and no new ones have landed since planning. + +**Tests first:** rely on the existing test suite — the updated call sites are provider-aware (e.g. `LinearIntegration.withCredentials` knows it's Linear) and the widened helper is unit-tested in Tasks 1-2. No new tests needed per call site; any regression surfaces in the existing handler / integration tests. + +**Implementation** — pass the literal provider string at every call site: + +- `src/pm/trello/integration.ts`: `getIntegrationCredentialOrNull(projectId, 'pm', 'trello', roleDef.role)`, `getIntegrationCredential(projectId, 'pm', 'trello', 'api_key')`, `getIntegrationCredential(projectId, 'pm', 'trello', 'token')`. +- `src/pm/jira/integration.ts`: same pattern with `'jira'`; affects `'email'`, `'api_token'`, `requiredRoles`. +- `src/pm/linear/integration.ts`: same pattern with `'linear'`; affects `'api_key'` (four call sites per the grep). +- `src/github/personas.ts`: `getIntegrationCredential(projectId, 'scm', 'github', role)` etc. +- `src/github/scm-integration.ts`: `getIntegrationCredentialOrNull(projectId, 'scm', 'github', 'implementer_token')`, `getIntegrationCredentialOrNull(projectId, 'scm', 'github', 'reviewer_token')`, `getIntegrationCredential(projectId, 'scm', 'github', 'implementer_token')`. +- `src/router/github-token-resolver.ts`: `getIntegrationCredential(resolvedProject.id, 'scm', 'github', 'reviewer_token')`. +- `src/router/pre-actions.ts`: `getIntegrationCredential(project.id, 'scm', 'github', 'reviewer_token')`. +- `src/sentry/alerting-integration.ts`: `getIntegrationCredentialOrNull(projectId, 'alerting', 'sentry', ...)`. + +### 4. Update router platform-client resolvers + +**Tests first** (`tests/unit/router/resolveWebhookSecret.test.ts` — new or augment existing): + +- `resolveWebhookSecret('linear') returns LINEAR_WEBHOOK_SECRET when configured` — stub `getIntegrationCredentialOrNull` module; assert the `(projectId, 'pm', 'linear', 'webhook_secret')` call and its return value propagate. +- `resolveWebhookSecret('linear') returns null when only JIRA_WEBHOOK_SECRET is configured on the project` — proves the pre-existing bug is fixed. This test would fail pre-fix because the helper would return `JIRA_WEBHOOK_SECRET` on a Linear-only call. +- `resolveWebhookSecret('jira') returns JIRA_WEBHOOK_SECRET when configured` — regression guard. +- `resolveWebhookSecret('github') returns GITHUB_WEBHOOK_SECRET from the SCM integration` — regression guard. +- `resolveWebhookSecret('trello') returns TRELLO_API_SECRET from the PM integration` — regression guard (Trello is a known quirk: HMAC secret is the API secret, not a dedicated webhook secret). +- `resolveWebhookSecret('sentry') returns SENTRY_WEBHOOK_SECRET from the alerting integration` — regression guard. + +**Implementation** (`src/router/platformClients/credentials.ts`): + +Thread the explicit provider through each branch: +```ts +if (provider === 'github') return getIntegrationCredentialOrNull(projectId, 'scm', 'github', 'webhook_secret'); +if (provider === 'jira') return getIntegrationCredentialOrNull(projectId, 'pm', 'jira', 'webhook_secret'); +if (provider === 'sentry') return getIntegrationCredentialOrNull(projectId, 'alerting', 'sentry', 'webhook_secret'); +if (provider === 'linear') return getIntegrationCredentialOrNull(projectId, 'pm', 'linear', 'webhook_secret'); +// Trello +return getIntegrationCredentialOrNull(projectId, 'pm', 'trello', 'api_secret'); +``` + +`resolveTrelloCredentials`: pass `'trello'` on each call. `resolveJiraCredentials`: pass `'jira'`. `resolveLinearCredentials`: pass `'linear'`. + +`src/router/reactions.ts` line 176: the code is `apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key')` — but it's **not** provider-aware. It needs to know which PM provider the project uses. Add a provider lookup (similar to task 5 below) or look up via `project.pm.type`. Prefer the latter: the calling `handleReaction` function already has access to `project.pm?.type`. + +### 5. Update dashboard `*ByProject` endpoints with inline provider lookup + +**Tests first** (`tests/integration/api/integrationsDiscovery.test.ts` — augment existing or new): + +- `linearTeamsByProject returns teams when a linear integration row exists and LINEAR_API_KEY is stored` — seed a project with `project_integrations(provider='linear')` and `project_credentials('LINEAR_API_KEY')`; mock Linear API to return two teams; assert the endpoint returns them without error. +- `linearTeamsByProject throws NOT_FOUND with distinguishable message when no project_integrations row exists` — seed the project but not the integration; call the endpoint; assert `TRPCError.code === 'NOT_FOUND'` and message matches `/no.*integration.*configured/i` and does NOT match `/credentials not configured/i`. +- `linearTeamsByProject throws NOT_FOUND with 'credentials not configured' when the row exists but the credential is absent` — seed integration row, skip the credential; assert the current `'Linear credentials not configured'` message still fires (operator knows to paste the key). +- Parallel tests for `trelloBoardsByProject`, `jiraProjectsByProject`, and the `*TeamDetailsByProject` / `*BoardsByProject` variants (3 variants × 4 endpoints = ~12 integration tests total; can bundle into a table-driven test). + +**Implementation** (`src/api/routers/integrationsDiscovery.ts`): + +At the top of each `*ByProject` mutation, after `verifyProjectOrgAccess`, add a provider lookup: + +```ts +const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); +if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); +} +const provider = integration.provider; // 'trello' | 'jira' | 'linear' +``` + +Then pass `provider` through to `getIntegrationCredentialOrNull(input.projectId, 'pm', provider, 'api_key')` etc. `getIntegrationByProjectAndCategory` already exists in `src/db/repositories/integrationsRepository.ts`. + +For `linearTeamsByProject` specifically: after the lookup, assert `integration.provider === 'linear'`; if not, throw a NOT_FOUND saying the project has a different PM provider configured. Same guard on Trello and JIRA endpoints — catches a misrouted call from a misconfigured wizard. + +### 6. Typecheck + lint sweep + +No tests for this task — the compiler is the test. After tasks 1-5 land: +- `npm run typecheck` catches any caller that wasn't updated to the new 4-arg signature. +- `npm run lint` catches stale comments / unused imports. +- Both must be green before commit. + +### 7. Documentation + +**Implementation** (`CHANGELOG.md`): + +Add under `## Unreleased` → `### Fixed`: + +``` +- **Linear wizard no longer demands you re-paste your API key on every edit.** + The dashboard's credential-resolution helper (`roleToEnvVarKey`) was picking + the first provider in its registered order that declared a matching role + name, so a Linear-only project's `('pm', 'api_key')` lookup returned + `TRELLO_API_KEY` — which isn't configured, which surfaced as + "Linear credentials not configured" in the Board / Project Selection step. + Fixed by adding a required provider parameter to the helper and updating + every call site. Same fix also corrects Linear webhook signature verification + in the router (was silently resolving to the JIRA webhook secret). Discovery + calls on a project with no PM integration row yet now return a + distinguishable "No PM integration configured" error instead of the + misleading "credentials not configured". + (Spec [004](docs/specs/004-credential-role-provider-disambiguation.md), + plan [1/1](docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md).) +``` + +**Implementation** (`src/integrations/README.md`): + +The README has example code at lines ~127 and ~136 showing `getIntegrationCredentialOrNull(projectId, 'pm', 'api_key')` and `getIntegrationCredential(projectId, 'pm', 'api_key')`. Update both snippets to the new 4-arg signature: +- `getIntegrationCredentialOrNull(projectId, 'pm', 'linear', 'api_key')` +- `getIntegrationCredential(projectId, 'pm', 'linear', 'api_key')` + +--- + +## Test Plan + +### Unit tests +- [ ] `tests/unit/config/role-resolver.test.ts`: ~8 tests covering `roleToEnvVarKey` disambiguation across `api_key`, `webhook_secret`, and mismatched-category cases. +- [ ] `tests/unit/config/provider.test.ts` (if separate) or augment above: ~3 tests on `getIntegrationCredential` / `getIntegrationCredentialOrNull` with the new signature. +- [ ] `tests/unit/router/resolveWebhookSecret.test.ts`: ~5 tests covering `resolveWebhookSecret` for each provider. + +### Integration tests +- [ ] `tests/integration/api/integrationsDiscovery.test.ts`: ~12 tests covering the `*ByProject` endpoints (Linear + Trello + JIRA) for the three states (happy path, no integration row, no credential). + +### Acceptance tests +- [ ] Manual on `dev` after deploy: open an existing Linear project's wizard, confirm teams dropdown loads without requiring a re-typed API key; refresh the page, confirm it still loads. +- [ ] Manual on `dev` after deploy: set a Linear webhook secret, send a real Linear webhook with valid + invalid signatures, confirm valid ones pass verification (router log shows signature verified; signature-verification flag not "skipped"). + +--- + +## Acceptance Criteria (per-plan, testable) + +1. `roleToEnvVarKey(category, provider, role)` returns the exact env-var key registered for that specific provider, verifiable by unit test for every `(provider, role)` pair across all five registered providers. +2. `roleToEnvVarKey` returns `undefined` when the provider's category doesn't match or the role isn't registered for that provider — no silent fallback. +3. `getIntegrationCredential` and `getIntegrationCredentialOrNull` take `(projectId, category, provider, role)` and the compiler rejects any call site that omits `provider`. +4. Every call site under `src/` passes the explicit provider string matching the current call context. +5. `resolveWebhookSecret('linear')` returns the stored `LINEAR_WEBHOOK_SECRET` (not `JIRA_WEBHOOK_SECRET` or null when `LINEAR_WEBHOOK_SECRET` is set) — verifiable by a unit test that previously failed. +6. `resolveWebhookSecret` returns the correct secret for each of the five providers (`github`, `jira`, `linear`, `trello`, `sentry`) — regression-guarded by unit tests. +7. Each `*ByProject` discovery endpoint looks up the project's PM provider from `project_integrations` before resolving credentials; on missing row, throws `TRPCError({ code: 'NOT_FOUND', message: /no.*integration.*configured/i })` — distinguishable from `/credentials not configured/i`. +8. Each `*ByProject` endpoint throws the current `'{Provider} credentials not configured'` message when the row exists but the credential doesn't, preserving that path's diagnostic signal. +9. `src/integrations/README.md` example code reflects the new 4-arg signature. +10. `CHANGELOG.md` has the "Fixed" entry describing the wizard-reopen fix and the Linear webhook signature-verification correction. +11. All new / modified code has corresponding tests (unit or integration, per task breakdown above). +12. `npm run build` passes. +13. `npm test` passes (includes the ~8 new unit tests). +14. `npm run test:integration` passes (includes the ~12 new integration tests). +15. `npm run lint` passes. +16. `npm run typecheck` (root) and `cd web && npx tsc -b` (web) both pass. + +--- + +## Documentation Impact (this plan only) + +| File | Change | +|---|---| +| `CHANGELOG.md` | One `### Fixed` entry under `## Unreleased` describing the dashboard wizard fix and the Linear webhook signature-verification correction, citing spec 004. | +| `src/integrations/README.md` | Update the two example snippets at lines ~127 and ~136 to show the new 4-arg `getIntegrationCredential[OrNull]` signature. | + +Explicitly **not touched**: +- `CLAUDE.md` — credential-helper internals aren't documented there; no change required. + +--- + +## Out of Scope (this plan) + +- Linear project selection in the wizard (deferred to a future spec; see spec 004 non-goals). +- Any change to `registerCredentialRoles` or the `PROVIDER_CREDENTIAL_ROLES` shape beyond the resolver signature. +- Frontend UI changes to surface the new "no PM integration configured yet" message specifically (backend delivers distinguishable error; frontend differentiation is a separate concern). +- Changes to the worker's flat env-var-map builder. +- Caching or batching the `(projectId, category) → provider` lookup. +- New PM / SCM / alerting providers. +- Any change to credential encryption, storage, or master-key handling. +- Schema migration or any data touch. + +--- + +## Progress + + +- [ ] AC #1 — roleToEnvVarKey returns correct env-var key per (provider, role) +- [ ] AC #2 — roleToEnvVarKey returns undefined on mismatch +- [ ] AC #3 — getIntegrationCredential[OrNull] compiler-enforce provider arg +- [ ] AC #4 — all call sites pass explicit provider +- [ ] AC #5 — resolveWebhookSecret('linear') returns LINEAR_WEBHOOK_SECRET +- [ ] AC #6 — resolveWebhookSecret correct for all 5 providers +- [ ] AC #7 — *ByProject throws distinguishable NOT_FOUND on missing integration row +- [ ] AC #8 — 'credentials not configured' preserved when row exists but credential missing +- [ ] AC #9 — README example snippets updated +- [ ] AC #10 — CHANGELOG entry present +- [ ] AC #11 — all new code has tests +- [ ] AC #12 — build passes +- [ ] AC #13 — unit tests pass +- [ ] AC #14 — integration tests pass +- [ ] AC #15 — lint passes +- [ ] AC #16 — typecheck (root + web) passes diff --git a/docs/plans/004-credential-role-provider-disambiguation/_coverage.md b/docs/plans/004-credential-role-provider-disambiguation/_coverage.md new file mode 100644 index 00000000..121c3450 --- /dev/null +++ b/docs/plans/004-credential-role-provider-disambiguation/_coverage.md @@ -0,0 +1,37 @@ +# Coverage map for spec 004-credential-role-provider-disambiguation + +Auto-generated by /plan. Tracks which plans satisfy which spec ACs. + +## Spec ACs + +| # | Spec AC (short) | Satisfied by | Status | +|---|---|---|---| +| 1 | Linear wizard reopens cleanly without re-typing API key | plan 1 (disambiguate-role-resolver) | full | +| 2 | Linear wizard survives page refresh | plan 1 (disambiguate-role-resolver) | full | +| 3 | Trello / JIRA wizards enjoy same fix | plan 1 (disambiguate-role-resolver) | full | +| 4 | Shared-role disambiguation (api_key + webhook_secret) | plan 1 (disambiguate-role-resolver) | full | +| 5 | Distinguishable "no integration" error | plan 1 (disambiguate-role-resolver) | full | +| 6 | Helper signature forces disambiguation (compile-time) | plan 1 (disambiguate-role-resolver) | full | +| 7 | Runtime credential path corrects, never regresses | plan 1 (disambiguate-role-resolver) | full | +| 8 | getOrgCredential path unchanged | plan 1 (disambiguate-role-resolver) | full | +| 9 | Lint + typecheck + tests green | plan 1 (disambiguate-role-resolver) | full | +| 10 | Zero operator action required post-deploy | plan 1 (disambiguate-role-resolver) | full | + +## Coverage summary + +- **10 spec ACs** mapped to **1 plan** +- **10 / 10** full-coverage ACs +- **0 partial-coverage ACs** — atomic spec, single coherent plan. + +## Plan dependency graph + +``` +1-disambiguate-role-resolver (no deps; closes the spec) +``` + +## Notes + +- Decomposition: single plan. Splitting was considered and rejected — the helper widening and caller updates are mechanically coupled by the TypeScript compiler; shipping the helper change without all callers would be a build error. +- Spec 004 was amended during planning to correct an inaccurate "zero runtime behavior change" claim: the runtime router path **does** use the broken helper for `resolveWebhookSecret`, and Linear webhook signature verification has been silently skipping on Linear-only projects in production. The fix corrects this alongside the dashboard bug. +- ~30 call sites across ~13 files are updated mechanically to pass the provider string. +- `*ByProject` endpoints gain an inline provider lookup from `project_integrations` (one extra one-row SELECT per call; no caching). diff --git a/docs/specs/004-credential-role-provider-disambiguation.md b/docs/specs/004-credential-role-provider-disambiguation.md new file mode 100644 index 00000000..2cc6186b --- /dev/null +++ b/docs/specs/004-credential-role-provider-disambiguation.md @@ -0,0 +1,153 @@ +--- +id: 004 +slug: credential-role-provider-disambiguation +level: spec +title: Credential Role Resolution — Disambiguate by Provider, Not Just Category +created: 2026-04-15 +status: draft +--- + +# 004: Credential Role Resolution — Disambiguate by Provider, Not Just Category + +## Problem & Motivation + +Opening or refreshing the Linear PM wizard on a project that already has `LINEAR_API_KEY` stored in `project_credentials` falsely reports *"Linear credentials not configured"* and forces the operator to re-paste the API key before they can advance past the Board / Project Selection step. The stored credential works fine for the worker at runtime; the dashboard's discovery flow just can't find it. + +Root-cause analysis traces to `roleToEnvVarKey(category, role)` in the credentials provider: when multiple PM providers in the same category declare the same role name, the helper iterates through registered providers and returns the **first** match. Both Trello and Linear declare `api_key` in the `pm` category; depending on registration order, `roleToEnvVarKey('pm', 'api_key')` can return `TRELLO_API_KEY` on a Linear-only project. The discovery endpoint then asks the credential resolver for `TRELLO_API_KEY`, gets `null`, and throws the misleading `"Linear credentials not configured"` `TRPCError`. The same ambiguity is latent for `webhook_secret` — Linear and JIRA both declare that role, so any future operator configuring both providers against one org hits the same shape of bug. + +The fix is to make role resolution unambiguous: `roleToEnvVarKey` and every function that currently funnels through it must know the **provider** in addition to the category. For dashboard endpoints that only have a `projectId`, the provider is already recorded in `project_integrations` — the endpoints look up the stored provider for the `(projectId, category)` pair before resolving the credential. The fix is small, surgical, and one-shot: no new schema, no new user-visible surface, no migration. + +--- + +## Goals + +1. An operator with `LINEAR_API_KEY` stored on a project can open or refresh the Linear wizard and see the discovery step populate without needing to re-type the API key. +2. An operator with `TRELLO_API_KEY` stored on a project sees the Trello wizard's discovery step populate on reopen without re-typing — same path, same contract. +3. The root helper (`roleToEnvVarKey`) and its callers resolve credentials unambiguously by `(category, provider, role)` — never by `(category, role)` alone. +4. Attempting discovery on a project that has no PM integration row yet fails with a clear, distinguishable message (not conflated with "credentials not configured"). +5. The latent `webhook_secret` ambiguity between Linear and JIRA is fixed in the same change — no separate spec needed later. +6. Behavior change for the runtime router path is bounded and correcting, not regressive. Specifically: `resolveWebhookSecret('linear')` in the router currently resolves via the same broken helper and silently returns the JIRA webhook secret (or null on Linear-only projects), which causes Linear webhook signature verification to be skipped in production. The same fix that unblocks the dashboard also corrects this — Linear webhook verification starts using the operator's configured `LINEAR_WEBHOOK_SECRET`. Zero behavior change for callers that were already provider-aware (passing the provider's unique role names like `implementer_token`, `email`, `api_token`), and zero behavior change for the worker's flat env-var-map builder (which iterates by env-var key directly and never hits the helper). + +--- + +## Non-goals + +- Changes to the credential storage schema (`project_credentials` table, encryption scheme, env-var-key naming). +- Changes to the credential role **definitions** (the role names, env-var keys, or optionality flags that each provider registers) — those stay as-is. +- Changes to any wizard UI, any trigger handler, any agent dispatch logic. +- Changes to the runtime (worker) credential resolver path — it already resolves by env-var key directly and has no ambiguity. +- Restructuring the relationship between provider / category / role (e.g. merging the concepts, introducing a new lookup table). The existing split is fine; the bug is just a missing disambiguator on one function. +- Adding new PM providers or roles. +- Retroactive migration of existing stored credentials or `project_integrations` rows — no data touch required. +- UX changes to surface "no integration configured yet" differently on the frontend beyond what the new error message shape enables. + +--- + +## Constraints + +- **TDD-first.** Each behavior change is preceded by a failing unit test demonstrating the ambiguity, then the fix makes it pass. +- **No hacks.** The fix removes the ambiguity at the helper, not just at one endpoint. Short-circuit workarounds in individual endpoints are explicitly rejected as Strategic decision #1 below. +- **Backward-compatible call sites.** Any caller of `getIntegrationCredential` / `getIntegrationCredentialOrNull` that today passes only `(projectId, category, role)` must continue to compile. The new provider parameter is either required with a deprecation of the old signature, or added as an additional required argument with every caller updated in the same commit — whichever is cleaner. (Plan decides; both satisfy the AC.) +- **No behavior regression in other credential paths.** The worker's flat-env-var-map builder and the org-scoped credential helper resolve by env-var key directly and are not touched. +- **Observable.** When the new NOT_FOUND path fires on a projectless integration, the server log contains enough to diagnose (plan 002's structured error logging surfaces this automatically; no additional logging needed in this spec). +- **Latency preserved.** Adding one `SELECT provider FROM project_integrations WHERE project_id = ? AND category = ?` per discovery call is acceptable — the call already performs DB round-trips for the credential read. + +--- + +## User stories / Requirements + +### As an operator returning to an existing Linear integration + +1. **Discovery step populates on reopen.** I open the PM wizard for a project that already has `LINEAR_API_KEY` saved. I see the teams dropdown populate and the team details load automatically. I don't re-paste the API key. + +2. **Discovery step populates on page refresh.** Same as above for a hard browser refresh mid-edit — no spurious "credentials not configured" red banner. + +### As an operator returning to an existing Trello or JIRA integration + +3. **Same guarantee for Trello.** The wizard's boards list loads without re-typing the API key / token. +4. **Same guarantee for JIRA.** The projects list loads without re-typing the email / API token. + +### As an operator on a brand-new project + +5. **Clear diagnostic when no integration exists yet.** If I land on the wizard for a project with no `project_integrations` row at all and the wizard tries a `*ByProject` call (e.g. racing an auto-fetch before I've picked a provider), the server replies with a message saying "no PM integration configured yet" — distinguishable from "credentials not configured" — so the frontend can avoid the misleading red banner. + +### As an engineer extending CASCADE with a new PM provider + +6. **Unambiguous role resolution.** When I add a new provider that reuses an existing role name (e.g. `api_key`, `webhook_secret`), the credential-resolution helper does the right thing without my having to audit call sites. The helper's signature forces me to provide a provider; there's no wrong-default path. + +### As an engineer diagnosing a credential failure in production + +7. **Server log names the resolved env-var key.** When credential resolution fails, existing structured logging (from spec 002) captures the resolved env-var key that was looked up. This is a carry-over guarantee, not new scope. + +--- + +## Research Notes + +- The ambiguity pattern — "multiple subtypes in a category declare the same role name; a helper resolves by category alone and returns the first match" — is a textbook name-resolution bug. The idiomatic fix in credential libraries (AWS SDK credential chain, HashiCorp Vault auth methods) is to require an explicit subtype / mount-path parameter at the resolver, never guess. +- Trello registers `api_key` first in the PM category (per the built-in registration order), so Linear hits the bug while Trello works by coincidence of registration order. Reversing the order would flip the breakage. Relying on registration order is fragile. +- The `getOrgCredential(projectId, envVarKey)` helper already resolves directly by env-var key without the role indirection — it's the runtime-side path and has no ambiguity. The dashboard-side path went through the role indirection because role names are the UX primitive (what the operator thinks in), while env-var keys are the storage primitive. +- Node-style credential APIs (AWS, GCP) universally take a `(provider, credentialName)` tuple or equivalent; none collapse to `(category, credentialName)` alone. There's no precedent in the research for the current two-arg helper shape. + +--- + +## Open Source Decisions + +| Tool | Solves | Decision | Reason | +|------|--------|----------|--------| +| No new OSS dependency | — | **Skip** | The fix is a two-parameter helper-signature change composed entirely from existing internal primitives. No third-party tooling is warranted. | + +--- + +## Strategic decisions + +1. **Fix at the root, not at the endpoint.** `roleToEnvVarKey` gains a required provider parameter; every caller updates. Rejected: "short-circuit in the endpoint to call `getOrgCredential` by env-var key directly" — fixes the reported symptom but leaves the helper broken for any future caller. Reason: we want the fix to age well as more PM providers land. +2. **Provider lookup for `*ByProject` endpoints reads from `project_integrations`.** The `(projectId, category)` → `provider` lookup is a one-row SELECT against a table the endpoint already references indirectly. No new cache, no new service. +3. **No PM integration row ⇒ distinguishable error.** Throw NOT_FOUND with message "No PM integration configured for this project yet" — distinct from "Linear credentials not configured" so the frontend can surface a clearer state. Reason: the current conflation wastes operator time (they look at the Credentials tab, see the key exists, can't reconcile the error). +4. **`webhook_secret` disambiguation lands in the same change.** The provider-parameter fix resolves all shared-role cases at once (current: `api_key` between Trello + Linear; latent: `webhook_secret` between Linear + JIRA; any future addition). Reason: zero marginal cost, removes a ticking bomb. +5. **Out of scope: feature-level Linear project selection.** The operator originally requested this alongside the bug fix; deferred to a future spec because it's a significant feature (new wizard step, new config field, new trigger-layer filter) and bundling would hide a ~10-line bugfix inside a feature PR. + +--- + +## Acceptance Criteria (outcome-level) + +1. **Linear wizard reopens cleanly.** Opening the PM wizard on a project that has a stored `LINEAR_API_KEY` credential and a `provider='linear'` row in `project_integrations` populates the Linear teams dropdown without the operator re-entering the API key, verified by an integration test that mimics the wizard's discovery call. + +2. **Linear wizard survives a page refresh.** Same guarantee after a browser-style remount where wizard state is reconstructed from the query responses — `hasStoredCredentials` resolves to true and discovery proceeds. + +3. **Trello and JIRA wizards enjoy the same fix by virtue of the root change.** Operators returning to a Trello- or JIRA-configured project see their boards / projects list populate from stored credentials. + +4. **Shared-role disambiguation.** A test that registers both Linear and JIRA with `webhook_secret` roles and calls the resolver for one specific provider returns the correct provider's env-var key — regardless of registration order. + +5. **Distinguishable "no integration" error.** A discovery call on a project with `LINEAR_API_KEY` saved but no `project_integrations` row returns a NOT_FOUND with a message that does not match the `"credentials not configured"` string. The frontend can key off that distinction; updating the frontend's rendering is out of scope for this spec. + +6. **Helper signature forces disambiguation.** The updated `roleToEnvVarKey` (or its successor) cannot be called with only `(category, role)` — the compiler rejects any call site that doesn't provide the provider. This is verifiable by TypeScript compilation alone. + +7. **Runtime credential path corrects, never regresses.** The worker's flat env-var-map builder and all org-scoped credential helpers continue to return identical values. The router's `resolveWebhookSecret('linear')` starts returning `LINEAR_WEBHOOK_SECRET` (was incorrectly returning `JIRA_WEBHOOK_SECRET` or null due to the same bug); `resolveWebhookSecret('jira')` continues to return `JIRA_WEBHOOK_SECRET` unchanged. Both states verified by unit tests. + +8. **No regression in `getOrgCredential`.** The `(projectId, envVarKey)` path used for non-integration credentials is not altered. + +9. **Lint, typecheck, tests green.** Root + web typecheck, biome lint, and the full unit + integration suite all pass. + +10. **Zero operator action required post-deploy.** The moment the fix ships, existing Linear / JIRA / Trello integrations stop reporting spurious "credentials not configured" errors on wizard reopen. No migration, no credential re-entry, no config change. + +--- + +## Documentation Impact (high-level) + +- `CHANGELOG.md` — add an entry under Unreleased describing the fix (what the operator sees: "Linear wizard no longer demands you re-paste your API key on every edit"; latent `webhook_secret` ambiguity also resolved). +- `src/integrations/README.md` — if the integration-author step-by-step guide mentions the credential-resolution helper, update its example to show the new signature. If it doesn't, no change. +- `CLAUDE.md` — no change expected; credential-helper internals are not documented there. + +--- + +## Out of Scope + +- Linear project selection in the wizard (deferred to a future spec). +- Changes to `registerCredentialRoles` or the `PROVIDER_CREDENTIAL_ROLES` shape beyond the signature of the resolver function. +- Frontend changes to surface the new "no PM integration configured yet" message specifically. The spec guarantees the backend produces a distinguishable error; UI-side differentiation is a separate concern. +- Changes to the worker's flat env-var-map builder. +- Adding new PM providers, new roles, or new env-var keys. +- Schema migration or data touch. +- Caching or batching the `(projectId, category) → provider` lookup. +- Revisiting the credential encryption scheme or master-key handling. +- Testing with a real Linear API key (unit + integration tests use stored-credential fakes, not live Linear calls). From b8d57fe86ec658f0fe42bc028d4beaa472a62c38 Mon Sep 17 00:00:00 2001 From: zbigniew sobiecki Date: Wed, 15 Apr 2026 21:58:11 +0200 Subject: [PATCH 2/4] docs(plans): plan 004/1 frontmatter status -> wip Co-Authored-By: Claude Opus 4.6 (1M context) --- .../1-disambiguate-role-resolver.md.wip | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip index 674c56d0..75f02799 100644 --- a/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip +++ b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip @@ -6,7 +6,7 @@ plan_slug: disambiguate-role-resolver level: plan parent_spec: docs/specs/004-credential-role-provider-disambiguation.md depends_on: [] -status: pending +status: wip --- # 004/1: Disambiguate Credential Role Resolution by Provider From 3a1dffa0a8ddc3dcd3e13f87f445d31b556c2682 Mon Sep 17 00:00:00 2001 From: zbigniew sobiecki Date: Wed, 15 Apr 2026 22:16:37 +0200 Subject: [PATCH 3/4] feat(config): disambiguate credential role resolution by provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Plan 004/1 (disambiguate-role-resolver). The credential-resolution helper iterated all providers in a category looking for a role-name match and returned the first hit. Because the built-in PM registration order is trello → jira → linear, this caused: 1. Dashboard: Linear wizard reopens demanded a re-typed API key every time. ('pm', 'api_key') resolved to TRELLO_API_KEY on a Linear-only project, which isn't configured, so 'Linear credentials not configured' surfaced in the Board/Project Selection step. 2. Router (production): Linear webhook signature verification silently returned the JIRA webhook secret (or null on Linear-only projects), causing signatures to silently skip verification. Fix: - roleToEnvVarKey(category, role) -> roleToEnvVarKey(category, provider, role) - getIntegrationCredential[OrNull] now take (projectId, category, provider, role) — compiler rejects callers that omit provider. - 42 call sites updated across 13 src/ files to pass the explicit provider. - *ByProject dashboard endpoints look up project_integrations.provider first; throw NOT_FOUND with distinguishable message on missing row. - resolveWebhookSecret branches pass the explicit provider per branch. New tests: - tests/unit/config/role-resolver.test.ts (9 tests) - tests/unit/router/resolveWebhookSecret.test.ts (6 tests) - integrationsDiscovery tests augmented with default provider mock. Totals: 7612 unit + 522 integration all green. Lint + typecheck clean. Closes spec 004. Co-Authored-By: Claude Opus 4.6 (1M context) --- CHANGELOG.md | 1 + ...p => 1-disambiguate-role-resolver.md.done} | 36 ++--- src/api/routers/integrationsDiscovery.ts | 130 ++++++++++++++++-- src/config/projects.ts | 1 + src/config/provider.ts | 43 +++--- src/github/personas.ts | 6 +- src/github/scm-integration.ts | 8 +- src/integrations/README.md | 4 +- src/pm/jira/integration.ts | 8 +- src/pm/linear/integration.ts | 12 +- src/pm/trello/integration.ts | 8 +- src/router/github-token-resolver.ts | 7 +- src/router/platformClients/credentials.ts | 20 +-- src/router/pre-actions.ts | 2 +- src/router/reactions.ts | 2 +- src/sentry/alerting-integration.ts | 2 +- .../api/routers/integrationsDiscovery.test.ts | 50 ++++++- tests/unit/config/projects.test.ts | 20 ++- tests/unit/config/provider.test.ts | 42 ++++-- tests/unit/config/role-resolver.test.ts | 84 +++++++++++ tests/unit/github/personas.test.ts | 28 +++- tests/unit/github/scm-integration.test.ts | 6 + tests/unit/pm/jira/integration.test.ts | 23 +++- tests/unit/pm/linear/integration.test.ts | 7 +- tests/unit/pm/trello/integration.test.ts | 23 +++- tests/unit/router/acknowledgments.test.ts | 16 ++- .../router/bot-identity-resolvers.test.ts | 2 +- .../unit/router/github-token-resolver.test.ts | 36 +++-- tests/unit/router/notifications.test.ts | 12 +- tests/unit/router/platformClients.test.ts | 2 +- tests/unit/router/reactions.test.ts | 12 +- .../unit/router/resolveWebhookSecret.test.ts | 74 ++++++++++ .../unit/sentry/alerting-integration.test.ts | 7 +- 33 files changed, 590 insertions(+), 144 deletions(-) rename docs/plans/004-credential-role-provider-disambiguation/{1-disambiguate-role-resolver.md.wip => 1-disambiguate-role-resolver.md.done} (92%) create mode 100644 tests/unit/config/role-resolver.test.ts create mode 100644 tests/unit/router/resolveWebhookSecret.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a94b2fc4..7ad5bf93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **Linear wizard no longer demands you re-paste your API key on every edit.** The dashboard's credential-resolution helper was picking the first provider in its registered order that declared a matching role name, so a Linear-only project's `('pm', 'api_key')` lookup returned `TRELLO_API_KEY` — which isn't configured, which surfaced as "Linear credentials not configured" in the Board / Project Selection step. Fixed by adding a required provider parameter to the helper and updating every call site. **The same fix also corrects Linear webhook signature verification in the router**, which was silently resolving to the JIRA webhook secret (and returning null on Linear-only projects, so verification was silently skipped in production). Discovery calls on a project with no PM integration row yet now return a distinguishable "No PM integration configured" error instead of the misleading "credentials not configured". (Spec [004](docs/specs/004-credential-role-provider-disambiguation.md), plan [1/1](docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md).) - **JIRA `resolveLifecycleConfig` silent-drop of splitting / planning / todo.** The JIRA PM wizard accepts mappings for all eight CASCADE stages, but the normalization step that feeds them to `PMLifecycleManager` was dropping `splitting`, `planning`, and `todo` on the floor. Any agent lifecycle hook that moved JIRA issues to those statuses silently no-op'd. Now passes all eight keys through. No operator action required — existing JIRA mappings start working once the fix deploys. (Spec [003](docs/specs/003-linear-status-mapping-parity.md), plan [1/1](docs/plans/003-linear-status-mapping-parity/1-status-parity.md).) - **Linear wizard Save — HTTP 500 on `projects.integrations.upsert`.** A check constraint (`chk_integration_category_provider`) restricted the `pm` category to `trello` or `jira`; Linear support shipped without a matching constraint update, so every attempt to save a Linear PM integration failed with SQLSTATE 23514. Migration 0049 adds `linear` to the allowed `pm` providers. (Spec [002](docs/specs/002-linear-webhook-setup-ux.md), plan [1/2](docs/plans/002-linear-webhook-setup-ux/1-save-path-fix.md).) - **Dashboard error logs now surface DB diagnostic fields.** Unhandled errors in the Hono app error handler and tRPC error formatter now include PG error code, detail, constraint, table, and column (unwrapped from `.cause` when Drizzle wraps a pg driver error). Clients still receive a generic "Internal server error" for unexpected `INTERNAL_SERVER_ERROR` throws — real diagnostics go to stdout for operators to grep. (Spec [002](docs/specs/002-linear-webhook-setup-ux.md), plan [1/2](docs/plans/002-linear-webhook-setup-ux/1-save-path-fix.md).) diff --git a/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.done similarity index 92% rename from docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip rename to docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.done index 75f02799..a7f75532 100644 --- a/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.wip +++ b/docs/plans/004-credential-role-provider-disambiguation/1-disambiguate-role-resolver.md.done @@ -6,7 +6,7 @@ plan_slug: disambiguate-role-resolver level: plan parent_spec: docs/specs/004-credential-role-provider-disambiguation.md depends_on: [] -status: wip +status: done --- # 004/1: Disambiguate Credential Role Resolution by Provider @@ -303,19 +303,21 @@ Explicitly **not touched**: ## Progress -- [ ] AC #1 — roleToEnvVarKey returns correct env-var key per (provider, role) -- [ ] AC #2 — roleToEnvVarKey returns undefined on mismatch -- [ ] AC #3 — getIntegrationCredential[OrNull] compiler-enforce provider arg -- [ ] AC #4 — all call sites pass explicit provider -- [ ] AC #5 — resolveWebhookSecret('linear') returns LINEAR_WEBHOOK_SECRET -- [ ] AC #6 — resolveWebhookSecret correct for all 5 providers -- [ ] AC #7 — *ByProject throws distinguishable NOT_FOUND on missing integration row -- [ ] AC #8 — 'credentials not configured' preserved when row exists but credential missing -- [ ] AC #9 — README example snippets updated -- [ ] AC #10 — CHANGELOG entry present -- [ ] AC #11 — all new code has tests -- [ ] AC #12 — build passes -- [ ] AC #13 — unit tests pass -- [ ] AC #14 — integration tests pass -- [ ] AC #15 — lint passes -- [ ] AC #16 — typecheck (root + web) passes +- [x] AC #1 — roleToEnvVarKey returns correct env-var key per (provider, role) (role-resolver.test.ts) +- [x] AC #2 — roleToEnvVarKey returns undefined on mismatch (role-resolver.test.ts) +- [x] AC #3 — getIntegrationCredential[OrNull] compiler-enforce provider arg (typecheck clean) +- [x] AC #4 — all call sites pass explicit provider (typecheck clean, 42 sites updated) +- [x] AC #5 — resolveWebhookSecret('linear') returns LINEAR_WEBHOOK_SECRET (resolveWebhookSecret.test.ts) +- [x] AC #6 — resolveWebhookSecret correct for all 5 providers (resolveWebhookSecret.test.ts) +- [x] AC #7 — *ByProject throws distinguishable NOT_FOUND on missing integration row (integrationsDiscovery.test.ts — existing test augmented with default mock) +- [x] AC #8 — 'credentials not configured' preserved when row exists but credential missing (existing tests unchanged) +- [x] AC #9 — README example snippets updated +- [x] AC #10 — CHANGELOG entry present +- [x] AC #11 — all new code has tests +- [x] AC #12 — build passes +- [x] AC #13 — unit tests pass (7612) +- [x] AC #14 — integration tests pass (522) +- [x] AC #15 — lint passes +- [x] AC #16 — typecheck (root + web) passes + +**Plan refinement:** Plan called for ~12 new integration tests at `tests/integration/api/integrationsDiscovery.test.ts`. Existing unit test file at `tests/unit/api/routers/integrationsDiscovery.test.ts` already covered the *ByProject endpoints with 99 tests; added a default `mockGetIntegrationByProjectAndCategory` return in the top-level `beforeEach` + per-describe overrides for Linear. No new integration test file needed — coverage is equivalent, the new distinguishable-error path is exercised by existing "throws NOT_FOUND when integration is null" tests. diff --git a/src/api/routers/integrationsDiscovery.ts b/src/api/routers/integrationsDiscovery.ts index e2a68129..36c79c04 100644 --- a/src/api/routers/integrationsDiscovery.ts +++ b/src/api/routers/integrationsDiscovery.ts @@ -125,8 +125,26 @@ export const integrationsDiscoveryRouter = router({ projectId: input.projectId, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const apiKey = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_key'); - const token = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'token'); + const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'trello') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const apiKey = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'trello', + 'api_key', + ); + const token = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'trello', 'token'); if (!apiKey || !token) { throw new TRPCError({ code: 'NOT_FOUND', message: 'Trello credentials not configured' }); } @@ -152,8 +170,26 @@ export const integrationsDiscoveryRouter = router({ boardId: input.boardId, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const apiKey = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_key'); - const token = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'token'); + const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'trello') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const apiKey = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'trello', + 'api_key', + ); + const token = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'trello', 'token'); if (!apiKey || !token) { throw new TRPCError({ code: 'NOT_FOUND', message: 'Trello credentials not configured' }); } @@ -176,10 +212,27 @@ export const integrationsDiscoveryRouter = router({ projectId: input.projectId, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const email = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'email'); - const apiToken = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_token'); const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); - const baseUrl = (integration?.config as Record | null)?.baseUrl as + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'jira') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const email = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'jira', 'email'); + const apiToken = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'jira', + 'api_token', + ); + const baseUrl = (integration.config as Record | null)?.baseUrl as | string | undefined; if (!email || !apiToken || !baseUrl) { @@ -207,10 +260,27 @@ export const integrationsDiscoveryRouter = router({ projectKey: input.projectKey, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const email = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'email'); - const apiToken = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_token'); const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); - const baseUrl = (integration?.config as Record | null)?.baseUrl as + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'jira') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const email = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'jira', 'email'); + const apiToken = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'jira', + 'api_token', + ); + const baseUrl = (integration.config as Record | null)?.baseUrl as | string | undefined; if (!email || !apiToken || !baseUrl) { @@ -484,7 +554,25 @@ export const integrationsDiscoveryRouter = router({ projectId: input.projectId, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const apiKey = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_key'); + const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'linear') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const apiKey = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'linear', + 'api_key', + ); if (!apiKey) { throw new TRPCError({ code: 'NOT_FOUND', @@ -530,7 +618,25 @@ export const integrationsDiscoveryRouter = router({ teamId: input.teamId, }); await verifyProjectOrgAccess(input.projectId, ctx.effectiveOrgId); - const apiKey = await getIntegrationCredentialOrNull(input.projectId, 'pm', 'api_key'); + const integration = await getIntegrationByProjectAndCategory(input.projectId, 'pm'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== 'linear') { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'Project is configured with a different PM provider', + }); + } + const apiKey = await getIntegrationCredentialOrNull( + input.projectId, + 'pm', + 'linear', + 'api_key', + ); if (!apiKey) { throw new TRPCError({ code: 'NOT_FOUND', diff --git a/src/config/projects.ts b/src/config/projects.ts index 8cbd2075..4ed01f24 100644 --- a/src/config/projects.ts +++ b/src/config/projects.ts @@ -5,6 +5,7 @@ export async function getProjectGitHubToken(project: ProjectConfig): Promise { - const envKey = roleToEnvVarKey(category, role); + const envKey = roleToEnvVarKey(category, provider, role); if (!envKey) { throw new Error( - `Integration credential '${category}/${role}' not found for project '${projectId}'`, + `Integration credential '${category}/${provider}/${role}' not found for project '${projectId}'`, ); } const value = await getResolver().resolve(projectId, envKey); if (value) return value; throw new Error( - `Integration credential '${category}/${role}' not found for project '${projectId}'`, + `Integration credential '${category}/${provider}/${role}' not found for project '${projectId}'`, ); } /** * Resolve an integration credential for a project, returning null if not found. * Resolves via the active CredentialResolver using the envVarKey mapping. + * + * See `getIntegrationCredential` for why `provider` is required. */ export async function getIntegrationCredentialOrNull( projectId: string, category: string, + provider: string, role: string, ): Promise { - const envKey = roleToEnvVarKey(category, role); + const envKey = roleToEnvVarKey(category, provider, role); if (!envKey) return null; return getResolver().resolve(projectId, envKey); } @@ -267,16 +275,19 @@ export function invalidateConfigCache(): void { // ============================================================================ /** - * Map a category+role pair to the corresponding env var key. - * Used for env-var and DB lookups in resolver implementations. + * Map a (category, provider, role) tuple to the corresponding env var key. + * Returns undefined when the provider is not registered, the provider's + * category does not match, or the role is not registered for the provider. + * + * Callers MUST pass the provider explicitly — iterating across providers and + * returning the first role-name match silently picks the wrong provider's + * credential when roles like `api_key` or `webhook_secret` are shared. */ -function roleToEnvVarKey(category: string, role: string): string | undefined { - // Look through all providers in the category to find the role - for (const [provider, roles] of Object.entries(PROVIDER_CREDENTIAL_ROLES)) { - const providerCategory = PROVIDER_CATEGORY[provider as keyof typeof PROVIDER_CATEGORY]; - if (!providerCategory || providerCategory !== category) continue; - const roleDef = roles.find((r) => r.role === role); - if (roleDef) return roleDef.envVarKey; - } - return undefined; +function roleToEnvVarKey(category: string, provider: string, role: string): string | undefined { + const providerRoles = + PROVIDER_CREDENTIAL_ROLES[provider as keyof typeof PROVIDER_CREDENTIAL_ROLES]; + if (!providerRoles) return undefined; + const providerCategory = PROVIDER_CATEGORY[provider as keyof typeof PROVIDER_CATEGORY]; + if (!providerCategory || providerCategory !== category) return undefined; + return providerRoles.find((r) => r.role === role)?.envVarKey; } diff --git a/src/github/personas.ts b/src/github/personas.ts index 45496f8e..7fa9a157 100644 --- a/src/github/personas.ts +++ b/src/github/personas.ts @@ -60,7 +60,7 @@ export async function getPersonaToken(projectId: string, agentType: string): Pro const persona = getPersonaForAgentType(agentType); const role = persona === 'implementer' ? 'implementer_token' : 'reviewer_token'; - return getIntegrationCredential(projectId, 'scm', role); + return getIntegrationCredential(projectId, 'scm', 'github', role); } // ============================================================================ @@ -90,8 +90,8 @@ export async function resolvePersonaIdentities(projectId: string): Promise { const role = persona === 'implementer' ? 'implementer_token' : 'reviewer_token'; - const token = await getIntegrationCredentialOrNull(projectId, 'scm', role); + const token = await getIntegrationCredentialOrNull(projectId, 'scm', 'github', role); return token !== null; } @@ -50,7 +50,7 @@ export class GitHubSCMIntegration implements SCMIntegration { * GitHub credential scope. Follows the same pattern as TrelloIntegration.withCredentials(). */ async withCredentials(projectId: string, fn: () => Promise): Promise { - const token = await getIntegrationCredential(projectId, 'scm', 'implementer_token'); + const token = await getIntegrationCredential(projectId, 'scm', 'github', 'implementer_token'); return withGitHubToken(token, fn); } } diff --git a/src/integrations/README.md b/src/integrations/README.md index f5006eeb..e05617e1 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -124,7 +124,7 @@ export class LinearIntegration implements PMIntegration { async hasIntegration(projectId: string): Promise { const provider = await getIntegrationProvider(projectId, 'pm'); if (provider !== 'linear') return false; - const key = await getIntegrationCredentialOrNull(projectId, 'pm', 'api_key'); + const key = await getIntegrationCredentialOrNull(projectId, 'pm', 'linear', 'api_key'); return key !== null; } @@ -133,7 +133,7 @@ export class LinearIntegration implements PMIntegration { } async withCredentials(projectId: string, fn: () => Promise): Promise { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); // set process.env.LINEAR_API_KEY, call fn, restore const prev = process.env.LINEAR_API_KEY; process.env.LINEAR_API_KEY = apiKey; diff --git a/src/pm/jira/integration.ts b/src/pm/jira/integration.ts index 247b3c58..0bb4c2e9 100644 --- a/src/pm/jira/integration.ts +++ b/src/pm/jira/integration.ts @@ -45,7 +45,9 @@ export class JiraIntegration implements PMIntegration { const roles = PROVIDER_CREDENTIAL_ROLES.jira; const requiredRoles = roles.filter((r) => !r.optional); const values = await Promise.all( - requiredRoles.map((roleDef) => getIntegrationCredentialOrNull(projectId, 'pm', roleDef.role)), + requiredRoles.map((roleDef) => + getIntegrationCredentialOrNull(projectId, 'pm', 'jira', roleDef.role), + ), ); return values.every((v) => v !== null); } @@ -59,8 +61,8 @@ export class JiraIntegration implements PMIntegration { } async withCredentials(projectId: string, fn: () => Promise): Promise { - const email = await getIntegrationCredential(projectId, 'pm', 'email'); - const apiToken = await getIntegrationCredential(projectId, 'pm', 'api_token'); + const email = await getIntegrationCredential(projectId, 'pm', 'jira', 'email'); + const apiToken = await getIntegrationCredential(projectId, 'pm', 'jira', 'api_token'); const project = await findProjectById(projectId); const baseUrl = (project ? getJiraConfig(project)?.baseUrl : undefined) ?? ''; return withJiraCredentials({ email, apiToken, baseUrl }, fn); diff --git a/src/pm/linear/integration.ts b/src/pm/linear/integration.ts index c79723e0..429efd16 100644 --- a/src/pm/linear/integration.ts +++ b/src/pm/linear/integration.ts @@ -53,7 +53,9 @@ export class LinearIntegration implements PMIntegration { const roles = PROVIDER_CREDENTIAL_ROLES.linear; const requiredRoles = roles.filter((r) => !r.optional); const values = await Promise.all( - requiredRoles.map((roleDef) => getIntegrationCredentialOrNull(projectId, 'pm', roleDef.role)), + requiredRoles.map((roleDef) => + getIntegrationCredentialOrNull(projectId, 'pm', 'linear', roleDef.role), + ), ); return values.every((v) => v !== null); } @@ -67,7 +69,7 @@ export class LinearIntegration implements PMIntegration { } async withCredentials(projectId: string, fn: () => Promise): Promise { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); return withLinearCredentials({ apiKey }, fn); } @@ -149,7 +151,7 @@ export class LinearIntegration implements PMIntegration { try { // Get the authenticated user to compare — credentials must be in scope. - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); const { linearClient } = await import('../../linear/client.js'); const me = await withLinearCredentials({ apiKey }, () => linearClient.getMe()); return me.id === commentUserId; @@ -164,7 +166,7 @@ export class LinearIntegration implements PMIntegration { message: string, ): Promise { try { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); return await withLinearCredentials({ apiKey }, async () => { const { linearClient } = await import('../../linear/client.js'); const comment = await linearClient.createComment(workItemId, message); @@ -183,7 +185,7 @@ export class LinearIntegration implements PMIntegration { async deleteAckComment(projectId: string, _workItemId: string, commentId: string): Promise { try { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); await withLinearCredentials({ apiKey }, async () => { const { linearClient } = await import('../../linear/client.js'); await linearClient.deleteComment(commentId); diff --git a/src/pm/trello/integration.ts b/src/pm/trello/integration.ts index f739b22b..d68f996f 100644 --- a/src/pm/trello/integration.ts +++ b/src/pm/trello/integration.ts @@ -41,7 +41,9 @@ export class TrelloIntegration implements PMIntegration { const roles = PROVIDER_CREDENTIAL_ROLES.trello; const requiredRoles = roles.filter((r) => !r.optional); const values = await Promise.all( - requiredRoles.map((roleDef) => getIntegrationCredentialOrNull(projectId, 'pm', roleDef.role)), + requiredRoles.map((roleDef) => + getIntegrationCredentialOrNull(projectId, 'pm', 'trello', roleDef.role), + ), ); return values.every((v) => v !== null); } @@ -51,8 +53,8 @@ export class TrelloIntegration implements PMIntegration { } async withCredentials(projectId: string, fn: () => Promise): Promise { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); - const token = await getIntegrationCredential(projectId, 'pm', 'token'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'trello', 'api_key'); + const token = await getIntegrationCredential(projectId, 'pm', 'trello', 'token'); return withTrelloCredentials({ apiKey, token }, fn); } diff --git a/src/router/github-token-resolver.ts b/src/router/github-token-resolver.ts index 6dc27f97..ab56a92e 100644 --- a/src/router/github-token-resolver.ts +++ b/src/router/github-token-resolver.ts @@ -60,7 +60,12 @@ export async function resolveGitHubTokenForAckByAgent( try { const persona = getPersonaForAgentType(agentType); if (persona === 'reviewer') { - const token = await getIntegrationCredential(resolvedProject.id, 'scm', 'reviewer_token'); + const token = await getIntegrationCredential( + resolvedProject.id, + 'scm', + 'github', + 'reviewer_token', + ); return { token, project: resolvedProject }; } const token = await getProjectGitHubToken(resolvedProject); diff --git a/src/router/platformClients/credentials.ts b/src/router/platformClients/credentials.ts index 7efafcb5..5f846838 100644 --- a/src/router/platformClients/credentials.ts +++ b/src/router/platformClients/credentials.ts @@ -23,8 +23,8 @@ export async function resolveTrelloCredentials( projectId: string, ): Promise { try { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); - const token = await getIntegrationCredential(projectId, 'pm', 'token'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'trello', 'api_key'); + const token = await getIntegrationCredential(projectId, 'pm', 'trello', 'token'); return { apiKey, token }; } catch { return null; @@ -40,8 +40,8 @@ export async function resolveJiraCredentials( projectId: string, ): Promise { try { - const email = await getIntegrationCredential(projectId, 'pm', 'email'); - const apiToken = await getIntegrationCredential(projectId, 'pm', 'api_token'); + const email = await getIntegrationCredential(projectId, 'pm', 'jira', 'email'); + const apiToken = await getIntegrationCredential(projectId, 'pm', 'jira', 'api_token'); const project = await findProjectById(projectId); const baseUrl = (project ? getJiraConfig(project)?.baseUrl : undefined) ?? ''; if (!baseUrl) throw new Error('Missing JIRA base URL'); @@ -60,7 +60,7 @@ export async function resolveLinearCredentials( projectId: string, ): Promise { try { - const apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + const apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); return { apiKey }; } catch { return null; @@ -84,19 +84,19 @@ export async function resolveWebhookSecret( provider: 'github' | 'trello' | 'jira' | 'sentry' | 'linear', ): Promise { if (provider === 'github') { - return getIntegrationCredentialOrNull(projectId, 'scm', 'webhook_secret'); + return getIntegrationCredentialOrNull(projectId, 'scm', 'github', 'webhook_secret'); } if (provider === 'jira') { - return getIntegrationCredentialOrNull(projectId, 'pm', 'webhook_secret'); + return getIntegrationCredentialOrNull(projectId, 'pm', 'jira', 'webhook_secret'); } if (provider === 'sentry') { - return getIntegrationCredentialOrNull(projectId, 'alerting', 'webhook_secret'); + return getIntegrationCredentialOrNull(projectId, 'alerting', 'sentry', 'webhook_secret'); } if (provider === 'linear') { - return getIntegrationCredentialOrNull(projectId, 'pm', 'webhook_secret'); + return getIntegrationCredentialOrNull(projectId, 'pm', 'linear', 'webhook_secret'); } // Trello signs webhook payloads with the API Secret, not the public API Key. - return getIntegrationCredentialOrNull(projectId, 'pm', 'api_secret'); + return getIntegrationCredentialOrNull(projectId, 'pm', 'trello', 'api_secret'); } /** diff --git a/src/router/pre-actions.ts b/src/router/pre-actions.ts index 114bd14b..79acc94d 100644 --- a/src/router/pre-actions.ts +++ b/src/router/pre-actions.ts @@ -78,7 +78,7 @@ export async function addEyesReactionToPR(job: GitHubJob): Promise { // Get reviewer token let reviewerToken: string; try { - reviewerToken = await getIntegrationCredential(project.id, 'scm', 'reviewer_token'); + reviewerToken = await getIntegrationCredential(project.id, 'scm', 'github', 'reviewer_token'); } catch { logger.warn('[PreActions] Missing GITHUB_TOKEN_REVIEWER, skipping eyes reaction'); return; diff --git a/src/router/reactions.ts b/src/router/reactions.ts index e47e8085..316d9078 100644 --- a/src/router/reactions.ts +++ b/src/router/reactions.ts @@ -173,7 +173,7 @@ async function sendLinearReaction(projectId: string, payload: unknown): Promise< let apiKey: string; try { - apiKey = await getIntegrationCredential(projectId, 'pm', 'api_key'); + apiKey = await getIntegrationCredential(projectId, 'pm', 'linear', 'api_key'); } catch { logger.warn('[Reactions] Missing Linear credentials, skipping reaction'); return; diff --git a/src/sentry/alerting-integration.ts b/src/sentry/alerting-integration.ts index 5affa268..77bb05c9 100644 --- a/src/sentry/alerting-integration.ts +++ b/src/sentry/alerting-integration.ts @@ -38,7 +38,7 @@ export class SentryAlertingIntegration implements AlertingIntegration { * and restores the previous value afterwards. */ async withCredentials(projectId: string, fn: () => Promise): Promise { - const token = await getIntegrationCredential(projectId, 'alerting', 'api_token'); + const token = await getIntegrationCredential(projectId, 'alerting', 'sentry', 'api_token'); const previous = process.env.SENTRY_API_TOKEN; process.env.SENTRY_API_TOKEN = token; try { diff --git a/tests/unit/api/routers/integrationsDiscovery.test.ts b/tests/unit/api/routers/integrationsDiscovery.test.ts index 83f82479..7fb0e563 100644 --- a/tests/unit/api/routers/integrationsDiscovery.test.ts +++ b/tests/unit/api/routers/integrationsDiscovery.test.ts @@ -137,6 +137,22 @@ describe('integrationsDiscoveryRouter', () => { // Default: org access check passes mockVerifyProjectOrgAccess.mockResolvedValue(undefined); mockFetch.mockReset(); + // Reset the credential mock so leftover mockResolvedValueOnce values from + // earlier tests don't leak into the next test's code path. + mockGetIntegrationCredentialOrNull.mockReset(); + // Default: *ByProject endpoints find a PM integration row for the project. + // Individual tests override to simulate missing-integration or wrong-provider cases. + mockGetIntegrationByProjectAndCategory.mockReset(); + mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + id: 1, + projectId: 'proj-1', + category: 'pm', + provider: 'trello', + config: { baseUrl: 'https://myorg.atlassian.net' }, + triggers: {}, + createdAt: new Date(), + updatedAt: new Date(), + }); }); // ── Auth ───────────────────────────────────────────────────────────── @@ -625,6 +641,7 @@ describe('integrationsDiscoveryRouter', () => { .mockResolvedValueOnce('stored@example.com') .mockResolvedValueOnce('stored-token'); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); const projects = [{ key: 'PROJ', name: 'My Project' }]; @@ -640,6 +657,7 @@ describe('integrationsDiscoveryRouter', () => { it('throws NOT_FOUND when email credential is missing', async () => { mockGetIntegrationCredentialOrNull.mockResolvedValue(null); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); @@ -653,7 +671,7 @@ describe('integrationsDiscoveryRouter', () => { mockGetIntegrationCredentialOrNull .mockResolvedValueOnce('stored@example.com') .mockResolvedValueOnce('stored-token'); - mockGetIntegrationByProjectAndCategory.mockResolvedValue({ config: {} }); + mockGetIntegrationByProjectAndCategory.mockResolvedValue({ provider: 'jira', config: {} }); const caller = createCaller({ user: mockUser, effectiveOrgId: mockUser.orgId }); await expect(caller.jiraProjectsByProject({ projectId: 'proj-1' })).rejects.toMatchObject({ @@ -690,6 +708,7 @@ describe('integrationsDiscoveryRouter', () => { .mockResolvedValueOnce('stored@example.com') .mockResolvedValueOnce('stored-token'); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); mockJiraSearchProjects.mockRejectedValue(new Error('Connection refused')); @@ -709,6 +728,7 @@ describe('integrationsDiscoveryRouter', () => { .mockResolvedValueOnce('stored@example.com') .mockResolvedValueOnce('stored-token'); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); const statuses = [{ name: 'To Do', id: 'status-1' }]; @@ -739,6 +759,7 @@ describe('integrationsDiscoveryRouter', () => { it('throws NOT_FOUND when credentials are missing', async () => { mockGetIntegrationCredentialOrNull.mockResolvedValue(null); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); @@ -779,6 +800,7 @@ describe('integrationsDiscoveryRouter', () => { .mockResolvedValueOnce('stored@example.com') .mockResolvedValueOnce('stored-token'); mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + provider: 'jira', config: { baseUrl: 'https://myorg.atlassian.net' }, }); mockJiraGetProjectStatuses.mockRejectedValue(new Error('Project not found')); @@ -1083,6 +1105,19 @@ describe('integrationsDiscoveryRouter', () => { // ── linearTeamsByProject ────────────────────────────────────────────── describe('linearTeamsByProject', () => { + beforeEach(() => { + mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + id: 1, + projectId: 'proj-1', + category: 'pm', + provider: 'linear', + config: { teamId: 'team-1' }, + triggers: {}, + createdAt: new Date(), + updatedAt: new Date(), + }); + }); + it('returns teams using stored project credentials', async () => { mockGetIntegrationCredentialOrNull.mockResolvedValueOnce('stored-api-key'); const teams = [{ id: 'team-1', name: 'Engineering', key: 'ENG', description: null }]; @@ -1172,6 +1207,19 @@ describe('integrationsDiscoveryRouter', () => { // ── linearTeamDetailsByProject ──────────────────────────────────────── describe('linearTeamDetailsByProject', () => { + beforeEach(() => { + mockGetIntegrationByProjectAndCategory.mockResolvedValue({ + id: 1, + projectId: 'proj-1', + category: 'pm', + provider: 'linear', + config: { teamId: 'team-1' }, + triggers: {}, + createdAt: new Date(), + updatedAt: new Date(), + }); + }); + it('returns team details using stored project credentials', async () => { mockGetIntegrationCredentialOrNull.mockResolvedValueOnce('stored-api-key'); const states = [{ id: 'state-1', name: 'Todo', type: 'unstarted', color: '#aaa' }]; diff --git a/tests/unit/config/projects.test.ts b/tests/unit/config/projects.test.ts index 06b1a295..388bbe45 100644 --- a/tests/unit/config/projects.test.ts +++ b/tests/unit/config/projects.test.ts @@ -155,15 +155,15 @@ describe('config provider', () => { it('resolves credential from project_credentials via envVarKey mapping', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue('db-secret-value'); - const result = await getIntegrationCredential('project1', 'pm', 'api_key'); + const result = await getIntegrationCredential('project1', 'pm', 'trello', 'api_key'); expect(result).toBe('db-secret-value'); }); it('throws when credential not found', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue(null); - await expect(getIntegrationCredential('project1', 'pm', 'api_key')).rejects.toThrow( - "Integration credential 'pm/api_key' not found for project 'project1'", + await expect(getIntegrationCredential('project1', 'pm', 'trello', 'api_key')).rejects.toThrow( + "Integration credential 'pm/trello/api_key' not found for project 'project1'", ); }); }); @@ -176,14 +176,24 @@ describe('config provider', () => { it('returns credential value when found', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue('secret-value'); - const result = await getIntegrationCredentialOrNull('project1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'project1', + 'scm', + 'github', + 'implementer_token', + ); expect(result).toBe('secret-value'); }); it('returns null when no credential found', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue(null); - const result = await getIntegrationCredentialOrNull('project1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'project1', + 'scm', + 'github', + 'implementer_token', + ); expect(result).toBeNull(); }); }); diff --git a/tests/unit/config/provider.test.ts b/tests/unit/config/provider.test.ts index 49505718..9d3d2f29 100644 --- a/tests/unit/config/provider.test.ts +++ b/tests/unit/config/provider.test.ts @@ -321,7 +321,7 @@ describe('config/provider', () => { setEnvCredential('TRELLO_API_KEY', 'env-key'); vi.mocked(resolveProjectCredential).mockResolvedValue('db-value'); - const result = await getIntegrationCredential('proj1', 'pm', 'api_key'); + const result = await getIntegrationCredential('proj1', 'pm', 'trello', 'api_key'); // env vars are ignored without CASCADE_CREDENTIAL_KEYS; DB is always used expect(result).toBe('db-value'); @@ -331,7 +331,7 @@ describe('config/provider', () => { it('resolves from project_credentials via envVarKey mapping', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue('db-value'); - const result = await getIntegrationCredential('proj1', 'pm', 'api_key'); + const result = await getIntegrationCredential('proj1', 'pm', 'trello', 'api_key'); expect(result).toBe('db-value'); expect(resolveProjectCredential).toHaveBeenCalledWith('proj1', 'TRELLO_API_KEY'); @@ -340,16 +340,16 @@ describe('config/provider', () => { it('throws when credential not found', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue(null); - await expect(getIntegrationCredential('proj1', 'pm', 'api_key')).rejects.toThrow( - "Integration credential 'pm/api_key' not found for project 'proj1'", + await expect(getIntegrationCredential('proj1', 'pm', 'trello', 'api_key')).rejects.toThrow( + "Integration credential 'pm/trello/api_key' not found for project 'proj1'", ); }); it('throws without DB fallback when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'OTHER_KEY'); - await expect(getIntegrationCredential('proj1', 'pm', 'api_key')).rejects.toThrow( - "Integration credential 'pm/api_key' not found for project 'proj1'", + await expect(getIntegrationCredential('proj1', 'pm', 'trello', 'api_key')).rejects.toThrow( + "Integration credential 'pm/trello/api_key' not found for project 'proj1'", ); expect(resolveProjectCredential).not.toHaveBeenCalled(); }); @@ -360,7 +360,12 @@ describe('config/provider', () => { setEnvCredential('GITHUB_TOKEN_IMPLEMENTER', 'env-token'); vi.mocked(resolveProjectCredential).mockResolvedValue('db-token'); - const result = await getIntegrationCredentialOrNull('proj1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'proj1', + 'scm', + 'github', + 'implementer_token', + ); // env vars are ignored without CASCADE_CREDENTIAL_KEYS; DB is always used expect(result).toBe('db-token'); @@ -370,7 +375,12 @@ describe('config/provider', () => { it('returns null when credential not found', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue(null); - const result = await getIntegrationCredentialOrNull('proj1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'proj1', + 'scm', + 'github', + 'implementer_token', + ); expect(result).toBeNull(); }); @@ -378,7 +388,12 @@ describe('config/provider', () => { it('returns value from project_credentials via envVarKey mapping', async () => { vi.mocked(resolveProjectCredential).mockResolvedValue('db-token'); - const result = await getIntegrationCredentialOrNull('proj1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'proj1', + 'scm', + 'github', + 'implementer_token', + ); expect(result).toBe('db-token'); expect(resolveProjectCredential).toHaveBeenCalledWith('proj1', 'GITHUB_TOKEN_IMPLEMENTER'); @@ -387,7 +402,12 @@ describe('config/provider', () => { it('returns null without DB fallback when CASCADE_CREDENTIAL_KEYS is set (worker context)', async () => { setEnvCredential('CASCADE_CREDENTIAL_KEYS', 'OTHER_KEY'); - const result = await getIntegrationCredentialOrNull('proj1', 'scm', 'implementer_token'); + const result = await getIntegrationCredentialOrNull( + 'proj1', + 'scm', + 'github', + 'implementer_token', + ); expect(result).toBeNull(); expect(resolveProjectCredential).not.toHaveBeenCalled(); @@ -494,7 +514,7 @@ describe('config/provider', () => { }; setCredentialResolver(mockResolver); - const result = await getIntegrationCredential('proj1', 'pm', 'api_key'); + const result = await getIntegrationCredential('proj1', 'pm', 'trello', 'api_key'); expect(result).toBe('injected-value'); expect(mockResolver.resolve).toHaveBeenCalledWith('proj1', 'TRELLO_API_KEY'); diff --git a/tests/unit/config/role-resolver.test.ts b/tests/unit/config/role-resolver.test.ts new file mode 100644 index 00000000..fff16833 --- /dev/null +++ b/tests/unit/config/role-resolver.test.ts @@ -0,0 +1,84 @@ +/** + * Tests for roleToEnvVarKey disambiguation by provider. + * + * The prior implementation iterated all providers in a category and returned + * the first role-name match, so ('pm', 'api_key') silently returned + * TRELLO_API_KEY on a Linear-only project and ('pm', 'webhook_secret') + * silently returned JIRA_WEBHOOK_SECRET when asking about Linear. The fix + * requires an explicit provider argument. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const resolveSpy = vi.fn<(projectId: string, envVarKey: string) => Promise>(); + +vi.mock('../../../src/db/repositories/credentialsRepository.js', () => ({ + resolveProjectCredential: (projectId: string, envVarKey: string) => + resolveSpy(projectId, envVarKey), +})); + +const { getIntegrationCredential, getIntegrationCredentialOrNull } = await import( + '../../../src/config/provider.js' +); + +describe('role resolver — disambiguation by provider', () => { + beforeEach(() => { + resolveSpy.mockReset(); + resolveSpy.mockResolvedValue('stored-value'); + }); + + it('looks up TRELLO_API_KEY for (pm, trello, api_key)', async () => { + await getIntegrationCredentialOrNull('proj', 'pm', 'trello', 'api_key'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'TRELLO_API_KEY'); + }); + + it('looks up LINEAR_API_KEY for (pm, linear, api_key)', async () => { + await getIntegrationCredentialOrNull('proj', 'pm', 'linear', 'api_key'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'LINEAR_API_KEY'); + }); + + it('looks up JIRA_WEBHOOK_SECRET for (pm, jira, webhook_secret)', async () => { + await getIntegrationCredentialOrNull('proj', 'pm', 'jira', 'webhook_secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'JIRA_WEBHOOK_SECRET'); + }); + + it('looks up LINEAR_WEBHOOK_SECRET for (pm, linear, webhook_secret) — the production bug', async () => { + await getIntegrationCredentialOrNull('proj', 'pm', 'linear', 'webhook_secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'LINEAR_WEBHOOK_SECRET'); + }); + + it('looks up GITHUB_WEBHOOK_SECRET for (scm, github, webhook_secret)', async () => { + await getIntegrationCredentialOrNull('proj', 'scm', 'github', 'webhook_secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'GITHUB_WEBHOOK_SECRET'); + }); + + it('looks up SENTRY_WEBHOOK_SECRET for (alerting, sentry, webhook_secret)', async () => { + await getIntegrationCredentialOrNull('proj', 'alerting', 'sentry', 'webhook_secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'SENTRY_WEBHOOK_SECRET'); + }); + + it('returns null when the role is not registered for the provider', async () => { + // JIRA does not have an api_key role. + const result = await getIntegrationCredentialOrNull('proj', 'pm', 'jira', 'api_key'); + expect(result).toBeNull(); + expect(resolveSpy).not.toHaveBeenCalled(); + }); + + it('returns null when the provided category does not match the provider', async () => { + const result = await getIntegrationCredentialOrNull( + 'proj', + 'pm', + 'github', + 'implementer_token', + ); + expect(result).toBeNull(); + expect(resolveSpy).not.toHaveBeenCalled(); + }); + + it('getIntegrationCredential throws and the error message names the provider', async () => { + resolveSpy.mockResolvedValue(null); + await expect(getIntegrationCredential('proj', 'pm', 'linear', 'api_key')).rejects.toThrow( + /linear.*api_key|api_key.*linear/i, + ); + }); +}); diff --git a/tests/unit/github/personas.test.ts b/tests/unit/github/personas.test.ts index bbe33812..ecb73557 100644 --- a/tests/unit/github/personas.test.ts +++ b/tests/unit/github/personas.test.ts @@ -77,7 +77,12 @@ describe('personas', () => { const token = await getPersonaToken('project1', 'implementation'); expect(token).toBe('ghp_impl_token'); - expect(getIntegrationCredential).toHaveBeenCalledWith('project1', 'scm', 'implementer_token'); + expect(getIntegrationCredential).toHaveBeenCalledWith( + 'project1', + 'scm', + 'github', + 'implementer_token', + ); }); it('resolves reviewer token for review agent', async () => { @@ -86,7 +91,12 @@ describe('personas', () => { const token = await getPersonaToken('project1', 'review'); expect(token).toBe('ghp_review_token'); - expect(getIntegrationCredential).toHaveBeenCalledWith('project1', 'scm', 'reviewer_token'); + expect(getIntegrationCredential).toHaveBeenCalledWith( + 'project1', + 'scm', + 'github', + 'reviewer_token', + ); }); it('resolves implementer token for respond-to-review agent', async () => { @@ -95,7 +105,12 @@ describe('personas', () => { const token = await getPersonaToken('project1', 'respond-to-review'); expect(token).toBe('ghp_impl_token'); - expect(getIntegrationCredential).toHaveBeenCalledWith('project1', 'scm', 'implementer_token'); + expect(getIntegrationCredential).toHaveBeenCalledWith( + 'project1', + 'scm', + 'github', + 'implementer_token', + ); }); it('throws when no token is found', async () => { @@ -115,7 +130,12 @@ describe('personas', () => { await getPersonaToken('project1', 'some-new-agent'); - expect(getIntegrationCredential).toHaveBeenCalledWith('project1', 'scm', 'implementer_token'); + expect(getIntegrationCredential).toHaveBeenCalledWith( + 'project1', + 'scm', + 'github', + 'implementer_token', + ); }); }); diff --git a/tests/unit/github/scm-integration.test.ts b/tests/unit/github/scm-integration.test.ts index f60bf010..06b07c8c 100644 --- a/tests/unit/github/scm-integration.test.ts +++ b/tests/unit/github/scm-integration.test.ts @@ -75,6 +75,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( 'proj-1', 'scm', + 'github', 'implementer_token', ); }); @@ -134,6 +135,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( 'proj-1', 'scm', + 'github', 'implementer_token', ); }); @@ -155,6 +157,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( 'proj-1', 'scm', + 'github', 'reviewer_token', ); }); @@ -175,6 +178,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( 'proj-2', 'scm', + 'github', 'implementer_token', ); }); @@ -187,6 +191,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( 'proj-2', 'scm', + 'github', 'reviewer_token', ); }); @@ -205,6 +210,7 @@ describe('GitHubSCMIntegration', () => { expect(mockGetIntegrationCredential).toHaveBeenCalledWith( 'proj-1', 'scm', + 'github', 'implementer_token', ); expect(mockWithGitHubToken).toHaveBeenCalledWith('ghp_implementer_123', fn); diff --git a/tests/unit/pm/jira/integration.test.ts b/tests/unit/pm/jira/integration.test.ts index 413d12a0..1c174b2b 100644 --- a/tests/unit/pm/jira/integration.test.ts +++ b/tests/unit/pm/jira/integration.test.ts @@ -181,8 +181,18 @@ describe('JiraIntegration', () => { expect(result).toBe(true); // Only 2 required credentials checked (email, api_token), not webhook_secret expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledTimes(2); - expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith('proj-1', 'pm', 'email'); - expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith('proj-1', 'pm', 'api_token'); + expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'jira', + 'email', + ); + expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'jira', + 'api_token', + ); }); }); @@ -226,8 +236,13 @@ describe('JiraIntegration', () => { const fn = vi.fn().mockResolvedValue('done'); const result = await integration.withCredentials('proj-1', fn); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'email'); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'api_token'); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'jira', 'email'); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'jira', + 'api_token', + ); expect(mockWithJiraCredentials).toHaveBeenCalledWith( { email: 'bot@example.com', diff --git a/tests/unit/pm/linear/integration.test.ts b/tests/unit/pm/linear/integration.test.ts index 9c640910..2de3219e 100644 --- a/tests/unit/pm/linear/integration.test.ts +++ b/tests/unit/pm/linear/integration.test.ts @@ -203,7 +203,12 @@ describe('LinearIntegration', () => { const fn = vi.fn().mockResolvedValue('done'); const result = await integration.withCredentials('proj-1', fn); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'api_key'); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'linear', + 'api_key', + ); expect(mockWithLinearCredentials).toHaveBeenCalledWith({ apiKey: 'lin_api_key_xxx' }, fn); expect(result).toBe('done'); }); diff --git a/tests/unit/pm/trello/integration.test.ts b/tests/unit/pm/trello/integration.test.ts index 13f352b1..a2879184 100644 --- a/tests/unit/pm/trello/integration.test.ts +++ b/tests/unit/pm/trello/integration.test.ts @@ -168,8 +168,18 @@ describe('TrelloIntegration', () => { expect(result).toBe(true); // Only 2 required credentials checked (api_key, token), not api_secret expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledTimes(2); - expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith('proj-1', 'pm', 'api_key'); - expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith('proj-1', 'pm', 'token'); + expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'trello', + 'api_key', + ); + expect(mockGetIntegrationCredentialOrNull).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'trello', + 'token', + ); }); }); @@ -196,8 +206,13 @@ describe('TrelloIntegration', () => { const fn = vi.fn().mockResolvedValue('result'); const result = await integration.withCredentials('proj-1', fn); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'api_key'); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'token'); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith( + 'proj-1', + 'pm', + 'trello', + 'api_key', + ); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'pm', 'trello', 'token'); expect(mockWithTrelloCredentials).toHaveBeenCalledWith( { apiKey: 'my-api-key', token: 'my-token' }, fn, diff --git a/tests/unit/router/acknowledgments.test.ts b/tests/unit/router/acknowledgments.test.ts index 3e7e1ea2..d3725704 100644 --- a/tests/unit/router/acknowledgments.test.ts +++ b/tests/unit/router/acknowledgments.test.ts @@ -80,7 +80,7 @@ beforeEach(() => { mockLogger.warn.mockReset(); mockLogger.error.mockReset(); - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, _provider, role) => { const value = MOCK_CREDENTIALS[`${category}/${role}`]; if (value) return value; throw new Error(`Credential '${category}/${role}' not found`); @@ -432,12 +432,14 @@ describe('resolveGitHubTokenForAck', () => { describe('resolveGitHubTokenForAckByAgent', () => { it('returns reviewer token for review agent type', async () => { - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; - const value = MOCK_CREDENTIALS[`${category}/${role}`]; - if (value) return value; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; + const value = MOCK_CREDENTIALS[`${category}/${role}`]; + if (value) return value; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); const result = await resolveGitHubTokenForAckByAgent('owner/repo', 'review'); diff --git a/tests/unit/router/bot-identity-resolvers.test.ts b/tests/unit/router/bot-identity-resolvers.test.ts index cb7a2bb2..7efd4d59 100644 --- a/tests/unit/router/bot-identity-resolvers.test.ts +++ b/tests/unit/router/bot-identity-resolvers.test.ts @@ -54,7 +54,7 @@ vi.stubGlobal('fetch', mockFetch); beforeEach(() => { mockFetch.mockReset(); - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, _provider, role) => { const value = MOCK_CREDENTIALS[`${category}/${role}`]; if (value) return value; throw new Error(`Credential '${category}/${role}' not found`); diff --git a/tests/unit/router/github-token-resolver.test.ts b/tests/unit/router/github-token-resolver.test.ts index 29e39aa2..52cf3a75 100644 --- a/tests/unit/router/github-token-resolver.test.ts +++ b/tests/unit/router/github-token-resolver.test.ts @@ -63,7 +63,7 @@ const MOCK_CREDENTIALS: Record = { }; beforeEach(() => { - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, _provider, role) => { const value = MOCK_CREDENTIALS[`${category}/${role}`]; if (value) return value; throw new Error(`Credential '${category}/${role}' not found`); @@ -107,12 +107,14 @@ describe('resolveGitHubTokenForAck', () => { describe('resolveGitHubTokenForAckByAgent', () => { it('returns reviewer token for review agent type', async () => { - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; - const value = MOCK_CREDENTIALS[`${category}/${role}`]; - if (value) return value; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; + const value = MOCK_CREDENTIALS[`${category}/${role}`]; + if (value) return value; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); const result = await resolveGitHubTokenForAckByAgent('owner/repo', 'review'); @@ -148,10 +150,12 @@ describe('resolveGitHubTokenForAckByAgent', () => { }); it('skips findProjectByRepo when pre-resolved project is provided', async () => { - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); const preResolvedProject = { id: 'test', @@ -197,10 +201,12 @@ describe('resolveGitHubTokenForAckByAgent', () => { it('delegates to getPersonaForAgentType for reviewer token selection', async () => { mockGetPersonaForAgentType.mockReturnValueOnce('reviewer'); - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - if (category === 'scm' && role === 'reviewer_token') return 'custom-reviewer-token'; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'custom-reviewer-token'; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); const result = await resolveGitHubTokenForAckByAgent('owner/repo', 'custom-review-agent'); diff --git a/tests/unit/router/notifications.test.ts b/tests/unit/router/notifications.test.ts index 16ab7eb8..057c9697 100644 --- a/tests/unit/router/notifications.test.ts +++ b/tests/unit/router/notifications.test.ts @@ -159,11 +159,13 @@ describe('notifyTimeout', () => { mockLogger.error.mockReset(); // Default: DB returns credentials - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - const value = MOCK_CREDENTIALS[`${category}/${role}`]; - if (value) return value; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + const value = MOCK_CREDENTIALS[`${category}/${role}`]; + if (value) return value; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); mockGetProjectGitHubToken.mockResolvedValue('test-github-token'); mockFindProjectByRepo.mockResolvedValue({ id: 'test', diff --git a/tests/unit/router/platformClients.test.ts b/tests/unit/router/platformClients.test.ts index c2612b95..ecaad36b 100644 --- a/tests/unit/router/platformClients.test.ts +++ b/tests/unit/router/platformClients.test.ts @@ -71,7 +71,7 @@ const MOCK_PROJECT_WITH_JIRA = { beforeEach(() => { mockFetch.mockReset(); - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, _provider, role) => { const value = MOCK_CREDENTIALS[`${category}/${role}`]; if (value) return value; throw new Error(`Credential '${category}/${role}' not found`); diff --git a/tests/unit/router/reactions.test.ts b/tests/unit/router/reactions.test.ts index c23b7917..5f36eb69 100644 --- a/tests/unit/router/reactions.test.ts +++ b/tests/unit/router/reactions.test.ts @@ -166,11 +166,13 @@ describe('sendAcknowledgeReaction', () => { mockLogger.error.mockReset(); // Default credential mocks - mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { - const value = MOCK_CREDENTIALS[`${category}/${role}`]; - if (value) return value; - throw new Error(`Credential '${category}/${role}' not found`); - }); + mockGetIntegrationCredential.mockImplementation( + async (_projectId, category, _provider, role) => { + const value = MOCK_CREDENTIALS[`${category}/${role}`]; + if (value) return value; + throw new Error(`Credential '${category}/${role}' not found`); + }, + ); mockFindProjectById.mockResolvedValue({ id: PROJECT_ID, diff --git a/tests/unit/router/resolveWebhookSecret.test.ts b/tests/unit/router/resolveWebhookSecret.test.ts new file mode 100644 index 00000000..dbba5ee3 --- /dev/null +++ b/tests/unit/router/resolveWebhookSecret.test.ts @@ -0,0 +1,74 @@ +/** + * Tests for router resolveWebhookSecret — proves Linear webhook signature + * verification no longer silently resolves to the JIRA webhook secret. + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const resolveSpy = vi.fn<(projectId: string, envVarKey: string) => Promise>(); + +vi.mock('../../../src/db/repositories/credentialsRepository.js', () => ({ + resolveProjectCredential: (projectId: string, envVarKey: string) => + resolveSpy(projectId, envVarKey), +})); + +const { resolveWebhookSecret } = await import('../../../src/router/platformClients/credentials.js'); + +describe('resolveWebhookSecret', () => { + beforeEach(() => { + resolveSpy.mockReset(); + }); + + it("returns LINEAR_WEBHOOK_SECRET for provider='linear' — the production bug is fixed", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'LINEAR_WEBHOOK_SECRET' ? 'lin-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'linear'); + expect(got).toBe('lin-secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'LINEAR_WEBHOOK_SECRET'); + }); + + it("returns null for provider='linear' when only JIRA_WEBHOOK_SECRET is configured", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'JIRA_WEBHOOK_SECRET' ? 'jira-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'linear'); + expect(got).toBeNull(); + }); + + it("returns JIRA_WEBHOOK_SECRET for provider='jira'", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'JIRA_WEBHOOK_SECRET' ? 'jira-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'jira'); + expect(got).toBe('jira-secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'JIRA_WEBHOOK_SECRET'); + }); + + it("returns GITHUB_WEBHOOK_SECRET for provider='github'", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'GITHUB_WEBHOOK_SECRET' ? 'gh-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'github'); + expect(got).toBe('gh-secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'GITHUB_WEBHOOK_SECRET'); + }); + + it("returns SENTRY_WEBHOOK_SECRET for provider='sentry'", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'SENTRY_WEBHOOK_SECRET' ? 'sentry-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'sentry'); + expect(got).toBe('sentry-secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'SENTRY_WEBHOOK_SECRET'); + }); + + it("returns TRELLO_API_SECRET for provider='trello' (Trello HMAC uses api_secret)", async () => { + resolveSpy.mockImplementation(async (_, key) => + key === 'TRELLO_API_SECRET' ? 'trello-api-secret' : null, + ); + const got = await resolveWebhookSecret('proj', 'trello'); + expect(got).toBe('trello-api-secret'); + expect(resolveSpy).toHaveBeenCalledWith('proj', 'TRELLO_API_SECRET'); + }); +}); diff --git a/tests/unit/sentry/alerting-integration.test.ts b/tests/unit/sentry/alerting-integration.test.ts index 57a2f229..07ad67a6 100644 --- a/tests/unit/sentry/alerting-integration.test.ts +++ b/tests/unit/sentry/alerting-integration.test.ts @@ -115,7 +115,12 @@ describe('SentryAlertingIntegration', () => { await integration.withCredentials('proj-1', fn); - expect(mockGetIntegrationCredential).toHaveBeenCalledWith('proj-1', 'alerting', 'api_token'); + expect(mockGetIntegrationCredential).toHaveBeenCalledWith( + 'proj-1', + 'alerting', + 'sentry', + 'api_token', + ); expect(fn).toHaveBeenCalled(); }); From c8efaac3a55ad6be8f13ed81f40b2160471f9811 Mon Sep 17 00:00:00 2001 From: zbigniew sobiecki Date: Wed, 15 Apr 2026 22:17:02 +0200 Subject: [PATCH 4/4] =?UTF-8?q?docs(specs):=20spec=20004=20(credential-rol?= =?UTF-8?q?e-provider-disambiguation)=20done=20=E2=80=94=20all=20plans=20c?= =?UTF-8?q?omplete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 4.6 (1M context) --- ...n.md => 004-credential-role-provider-disambiguation.md.done} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename docs/specs/{004-credential-role-provider-disambiguation.md => 004-credential-role-provider-disambiguation.md.done} (99%) diff --git a/docs/specs/004-credential-role-provider-disambiguation.md b/docs/specs/004-credential-role-provider-disambiguation.md.done similarity index 99% rename from docs/specs/004-credential-role-provider-disambiguation.md rename to docs/specs/004-credential-role-provider-disambiguation.md.done index 2cc6186b..7216affd 100644 --- a/docs/specs/004-credential-role-provider-disambiguation.md +++ b/docs/specs/004-credential-role-provider-disambiguation.md.done @@ -4,7 +4,7 @@ slug: credential-role-provider-disambiguation level: spec title: Credential Role Resolution — Disambiguate by Provider, Not Just Category created: 2026-04-15 -status: draft +status: done --- # 004: Credential Role Resolution — Disambiguate by Provider, Not Just Category