From ca713d232725d3ae7b2f49c22e69f44b54694181 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 12:35:55 +0200 Subject: [PATCH 01/15] fix(web): guard PM wizard hook architecture (#1310) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(web): guard PM wizard hook architecture * fix(web): widen providerId to string in shared mutation config types LabelCreationConfig, CustomFieldCreationConfig, and runPerLabelCreations previously typed providerId as a literal union of the current production providers ('trello' | 'linear' / 'trello' | 'jira'). A new PM provider declaring createLabel or createCustomField would need to edit pm-wizard-hooks.ts to add its id to those unions, breaking the new-provider/no-shared-hook-edit guarantee. Changed all three to string so the backend manifest capability check is the sole gate. Added an architecture-guard test assertion to pin this invariant. Co-Authored-By: Claude Sonnet 4.6 * fix(web): add frontend provider barrel for pm-wizard registration Create web/src/components/projects/pm-providers/index.ts as the symmetric counterpart of the backend barrel at src/integrations/pm/index.ts. This gives new PM providers a concrete registration path — add one import to this barrel — without ever editing pm-wizard.tsx. Updates pm-wizard.tsx to import the barrel instead of three individual provider modules, and updates README, CLAUDE.md, and the new-provider-surface guard test to accurately document the two-barrel (backend + frontend) pattern. Closes the docs/reality gap flagged in review: the guide now says new providers add to the frontend barrel, not that pm-wizard.tsx gets zero edits via some non-existent auto-discovery mechanism. Co-Authored-By: Claude Sonnet 4.6 * fix(web): make PM wizard provider picker registry-driven The provider picker was still hardcoded to ['trello', 'jira', 'linear'] with a static PROVIDER_LABELS map, meaning a new provider added to the frontend barrel (web/src/components/projects/pm-providers/index.ts) would register successfully but never appear as a selectable option in the dashboard wizard picker. Replace the hardcoded array with listProviderWizards() from the registry and drive labels from each wizard definition's .label field. confirmProviderSwitch now accepts plain strings instead of the literal union so it's fully generic. This closes the gap flagged by the reviewer: the picker now automatically includes any provider registered via the barrel, making the "zero pm-wizard.tsx edits for a new provider" guarantee actually hold. Co-Authored-By: Claude Sonnet 4.6 * fix(web): make PM wizard credential-readiness and auth paths metadata-driven - Widen `Provider` type from closed union to `string` so new providers registered via the frontend barrel don't need a hack `as Provider` cast - Add `areCredentialsReadyFromMetadata` to pm-wizard-hooks.ts driven by the provider's `auth.rawCredentials` spec; replaces the Trello/JIRA/Linear switch-fallthrough in `areCredentialsReady` - Replace `shouldUseStoredCredentials(state)` call in `buildProviderAuthArgFromMetadata` with a provider-agnostic inline check (`state.isEditing && state.hasStoredCredentials`) so mutation auth works for any registered provider without shared-file edits - Update CLAUDE.md, AGENTS.md, and src/integrations/README.md to honestly document that `pm-wizard-state.ts` still requires credential-field additions for new providers, while the readiness and auth paths are now metadata-driven Co-Authored-By: Claude Sonnet 4.6 * docs: make pm-wizard-state.ts exception explicit in new-provider docs The frontend barrel comment and README step-by-step guide both claimed that the barrel import was the only shared frontend edit needed for a new PM provider. This was misleading because pm-wizard-state.ts still requires manual edits (credential fields in WizardState, action types in WizardAction, and a buildEditState branch). Changes: - Narrow the barrel comment: replace "No other shared file needs to change" with an explicit callout of pm-wizard-state.ts and what each new provider must add there. - Add step 4 in README "Adding a new PM provider" that spells out the three pm-wizard-state.ts edits, with a note that areCredentialsReady FromMetadata and buildProviderAuthArgFromMetadata are metadata-driven and require no changes there. - Renumber steps 4→5, 5→6, 6→7, 7→8. - Update the intro paragraph and closing summary to acknowledge pm-wizard-state.ts as the deliberate shared-dashboard exception while keeping the no-pm-wizard.tsx / no-pm-wizard-hooks.ts guarantee clear. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Sonnet 4.6 --- CLAUDE.md | 2 +- src/integrations/README.md | 25 ++- .../integrations/new-provider-surface.test.ts | 8 +- .../web/pm-wizard-hooks-architecture.test.ts | 86 ++++++++++ tests/unit/web/pm-wizard-hooks.test.ts | 155 ++---------------- .../components/projects/pm-providers/index.ts | 23 +++ .../projects/pm-providers/jira/auth.ts | 16 ++ .../projects/pm-providers/jira/hooks.ts | 2 + .../projects/pm-providers/jira/wizard.ts | 17 +- .../projects/pm-providers/linear/auth.ts | 11 ++ .../projects/pm-providers/linear/hooks.ts | 2 + .../projects/pm-providers/linear/wizard.ts | 12 +- .../projects/pm-providers/trello/auth.ts | 15 ++ .../projects/pm-providers/trello/hooks.ts | 3 + .../projects/pm-providers/trello/wizard.ts | 16 +- .../components/projects/pm-providers/types.ts | 6 + .../components/projects/pm-wizard-hooks.ts | 95 ++++------- .../components/projects/pm-wizard-state.ts | 15 +- web/src/components/projects/pm-wizard.tsx | 53 +++--- 19 files changed, 281 insertions(+), 281 deletions(-) create mode 100644 tests/unit/web/pm-wizard-hooks-architecture.test.ts create mode 100644 web/src/components/projects/pm-providers/index.ts create mode 100644 web/src/components/projects/pm-providers/jira/auth.ts create mode 100644 web/src/components/projects/pm-providers/linear/auth.ts create mode 100644 web/src/components/projects/pm-providers/trello/auth.ts diff --git a/CLAUDE.md b/CLAUDE.md index 6134725f0..661bb6bb3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ Flow: `PM/SCM/alerting webhook → Router → Redis → Worker → TriggerRegist **Capacity-gate invariant.** Every PM router adapter (`src/router/adapters/{linear,trello,jira}.ts`) must wrap `triggerRegistry.dispatch(ctx)` in PM-provider `AsyncLocalStorage` scope via the shared `withPMScopeForDispatch(fullProject, dispatch)` helper at `src/router/adapters/_shared.ts` — in addition to the per-PM-type credential scope (`withLinearCredentials` / `withTrelloCredentials` / `withJiraCredentials`). Without the PM-provider wrapping, the pipeline-capacity gate at `src/triggers/shared/pipeline-capacity-gate.ts` cannot resolve `getPMProvider()`, **fails closed** under the spec-017 fail-closed policy (blocks the run + ERROR + Sentry capture under tag `pipeline_capacity_gate_no_pm_provider`), and `maxInFlightItems` is silently disabled for the PM-source path. Mirror the GitHub adapter's existing correct shape at `src/router/adapters/github.ts:dispatchWithCredentials`. The static guard at `tests/unit/integrations/pm-router-adapter-pm-scope.test.ts` enforces this at CI time — adding a new PM router adapter without the wrapping fails CI with a precise file path. -Integration abstraction lives in `src/integrations/`. For **adding a new PM provider**, see @src/integrations/README.md — PM providers (Trello, JIRA, Linear) use the `PMProviderManifest` registry with a **behavioral conformance harness** (spec 009 — config round-trip, discovery shape, full lifecycle scenario, auth-header provenance, single-entrypoint invariant). Each provider owns its Zod config schema (`src/integrations/pm//config-schema.ts`) as the single source of truth — the central `src/config/schema.ts` imports it. PM adapter method signatures use branded `StateId` / `LabelId` / `ContainerId` from `src/pm/ids.ts` to make state-name-vs-ID confusion a compile error at direct-adapter call sites. All runtime surfaces (router, worker, CLI, dashboard) register integrations through a single entrypoint at `src/integrations/entrypoint.ts`. **Spec 010 follow-ups** added generic `pm.discovery.createLabel` / `createCustomField` mutation endpoints + `currentUser` discovery capability + real shared React components for every `StandardStepKind` under `web/src/components/projects/pm-providers/steps/`. **Spec 011** migrated all three production providers (Trello, JIRA, Linear) onto those shared components, added a 7th `StandardStepKind: custom-field-mapping`, widened `container-pick` / `project-scope` / `webhook-url-display` with optional props, and deleted the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` files. **Spec 012** migrated each provider's webhook UX (programmatic create for Trello/JIRA, signing-secret + instructions for Linear) into per-provider manifest webhook adapters (Fragment compositions around the shared `WebhookUrlDisplayStep`); deleted the legacy `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo`. Every PM wizard step now renders via the manifest path without exception. A new PM provider writes zero edits to shared orchestration (`pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, `pm-wizard-hooks.ts`); provider-specific UI ships either as `kind: 'custom'` steps or as Fragment compositions inside the provider folder's wizard adapters. SCM (GitHub) and alerting (Sentry) still use the legacy `IntegrationModule` pattern via self-registration in `src/github/register.ts` + `src/sentry/register.ts`. Don't improvise; the README covers both patterns. +Integration abstraction lives in `src/integrations/`. For **adding a new PM provider**, see @src/integrations/README.md — PM providers (Trello, JIRA, Linear) use the `PMProviderManifest` registry with a **behavioral conformance harness** (spec 009 — config round-trip, discovery shape, full lifecycle scenario, auth-header provenance, single-entrypoint invariant). Each provider owns its Zod config schema (`src/integrations/pm//config-schema.ts`) as the single source of truth — the central `src/config/schema.ts` imports it. PM adapter method signatures use branded `StateId` / `LabelId` / `ContainerId` from `src/pm/ids.ts` to make state-name-vs-ID confusion a compile error at direct-adapter call sites. All runtime surfaces (router, worker, CLI, dashboard) register integrations through a single entrypoint at `src/integrations/entrypoint.ts`. **Spec 010 follow-ups** added generic `pm.discovery.createLabel` / `createCustomField` mutation endpoints + `currentUser` discovery capability + real shared React components for every `StandardStepKind` under `web/src/components/projects/pm-providers/steps/`. **Spec 011** migrated all three production providers (Trello, JIRA, Linear) onto those shared components, added a 7th `StandardStepKind: custom-field-mapping`, widened `container-pick` / `project-scope` / `webhook-url-display` with optional props, and deleted the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` files. **Spec 012** migrated each provider's webhook UX (programmatic create for Trello/JIRA, signing-secret + instructions for Linear) into per-provider manifest webhook adapters (Fragment compositions around the shared `WebhookUrlDisplayStep`); deleted the legacy `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo`. Every PM wizard step now renders via the manifest path without exception. A new PM provider writes one import in the backend barrel (`src/integrations/pm/index.ts`) and one import in the frontend barrel (`web/src/components/projects/pm-providers/index.ts`); `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, and `pm-wizard-hooks.ts` receive zero edits. The verification-button readiness path (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are metadata-driven and require no changes for a new provider. **The shared dashboard state (`pm-wizard-state.ts`) does still require edits**: new providers must add their credential fields to `WizardState` (e.g. `asanaApiKey: string`), the corresponding action types to `WizardAction`, and the config-shape handling in `buildEditState` — see step 3 of @src/integrations/README.md. Provider-specific hooks, auth metadata, verification display formatting, and UI live inside the provider folder (`kind: 'custom'` steps or Fragment compositions around shared steps). SCM (GitHub) and alerting (Sentry) still use the legacy `IntegrationModule` pattern via self-registration in `src/github/register.ts` + `src/sentry/register.ts`. Don't improvise; the README covers both patterns. ## PR checkout (worker) — gotcha diff --git a/src/integrations/README.md b/src/integrations/README.md index 4b6aef3fd..fde166858 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -204,13 +204,13 @@ All three real providers are now on the hardened contracts. Plan 009/4 also ship | Webhook-UX migration complete | Every PM wizard step, without exception, renders via the manifest path. Trello, JIRA, and Linear each own their webhook step via a per-provider adapter (`pm-providers//webhook-step.tsx`) — Fragment composition around the shared `WebhookUrlDisplayStep`. Trello + JIRA compose with programmatic "Create Webhook" button + active-webhooks list + delete + curl fallback (via existing `webhooks.create/list/delete({trelloOnly|jiraOnly:true})` tRPC endpoints). Linear composes with info banner + `ProjectSecretField` (`LINEAR_WEBHOOK_SECRET`) + 5-step manual setup instructions. | | Legacy deletions | `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo` all deleted. `pm-wizard-common-steps.tsx` now only exports `SaveStep`. Legacy test file `pm-wizard-webhooks-step.test.ts` deleted — assertions moved into per-provider adapter tests. | | Parent-wizard filter | The `-webhook` id-skip filter (stopgap from plan 011/4) is gone. `renderedManifestSteps = manifestDef.steps.map(...)` — no filter. | -| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. Webhook UX composition lives in the provider folder. | +| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. New providers add one import to the frontend barrel (`web/src/components/projects/pm-providers/index.ts`) — the symmetric counterpart of the backend barrel — and `pm-wizard.tsx` picks it up automatically. The provider picker, verification-button readiness (`areCredentialsReadyFromMetadata`), and mutation auth path (`buildProviderAuthArgFromMetadata`) are all metadata-driven; no shared edits required beyond the barrel import. **Shared dashboard state** (`pm-wizard-state.ts`) must still be updated with the new provider's credential fields (`WizardState`), reducer action types, and `buildEditState` handling — see step 3 of "Adding a new PM provider" below. | --- ## Adding a new PM provider (step by step) -Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. Everything lives in your provider folder + your wizard folder + a single import in `src/integrations/pm/index.ts`. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces this. +Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. The orchestration and schema work lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The one shared dashboard file that still requires a new-provider edit is `pm-wizard-state.ts` (step 4 below) — new providers add credential fields to `WizardState`, action types to `WizardAction`, and a `buildEditState` branch. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces the shared-file invariant for orchestration and schema files; `pm-wizard-state.ts` is the deliberate exception. 1. **Backend folder** at `src/integrations/pm//`: - `client.ts` (or reuse a sibling under `src//`) — your REST / GraphQL client. Must use `withXxxCredentials()` + AsyncLocalStorage credential scoping; never hand-assemble Bearer headers (see `_shared/auth-headers.ts`). @@ -220,17 +220,26 @@ Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / - `router-adapter.ts`, `triggers/*.ts`, `webhook.ts`, `platform-client.ts` — same as before. - `index.ts` — side-effect module calling `registerPMProvider(Manifest)`. -2. **Wire the manifest** via a single import in `src/integrations/pm/index.ts` (`import './/index.js';`). No other edit to any shared file is needed for registration — the `single-entrypoint` test guards this. +2. **Wire the backend manifest** via a single import in `src/integrations/pm/index.ts` (`import './/index.js';`). No other backend file needs to change — the `single-entrypoint` test guards this. -3. **Frontend folder** at `web/src/components/projects/pm-providers//`: `wizard.ts` (`ProviderWizardDefinition` with `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `index.ts`. Add one line to `pm-wizard.tsx` to register. For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). +3. **Frontend folder** at `web/src/components/projects/pm-providers//`: `wizard.ts` (`ProviderWizardDefinition` with `auth`, `credentialPersistence`, `formatVerificationDisplay`, and `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `hooks.ts` for provider-owned discovery/mutation wrappers, `auth.ts` for reusable auth metadata, and `index.ts` for side-effect registration (`registerProviderWizard(ProviderWizard)`). For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). Shared `pm-wizard-hooks.ts` remains limited to metadata-driven verification/save shells and provider-agnostic mutation factories. -4. **Lifecycle fixture** at `tests/helpers/LifecycleFixture.ts`. Add the fixture key to `LIFECYCLE_FIXTURES` in `tests/unit/integrations/pm-conformance.test.ts`. Trivial providers can reuse `createFakePMProvider()` (see Trello/JIRA/Linear fixtures). +4. **Update shared dashboard state** in `web/src/components/projects/pm-wizard-state.ts`. This is the one shared dashboard file a new provider must edit: + - Add the provider's raw credential fields to `WizardState` (e.g. `asanaApiKey: string`). + - Add the corresponding `SET__` action types to `WizardAction` (one per credential field); each reducer case should set `verificationResult: null, verifyError: null` alongside the updated field. + - Add an `else if (provider === '')` branch to `buildEditState` that sets `hasStoredCredentials` based on `configuredKeys.has('')` and restores any saved config values (container IDs, status/label mappings). -5. **Run the conformance harness**: `npx vitest run --project unit-core tests/unit/integrations/pm-conformance.test.ts`. Behavioral contracts run against your provider automatically once `configSchema` / `discoveryCapabilities` / `lifecycle` are declared. Failures name the contract. + The credential-readiness check (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are fully metadata-driven — they read `manifestDef.auth.rawCredentials` and require **no changes** here. -6. **Provider-specific unit tests** in `tests/unit/pm//` — adapter tests (vi.mock the client), config-schema round-trip, discovery shape, wizardSpec, adapter branded IDs. +5. **Wire the frontend wizard** via a single import in `web/src/components/projects/pm-providers/index.ts` (`import './/index.js';`). This frontend barrel is the symmetric counterpart of the backend barrel at `src/integrations/pm/index.ts` — `pm-wizard.tsx` imports the barrel once and never needs to be edited for a new provider. -That's it. The `new-provider-surface` snapshot test proves your PR touches **no** shared infrastructure file. +6. **Lifecycle fixture** at `tests/helpers/LifecycleFixture.ts`. Add the fixture key to `LIFECYCLE_FIXTURES` in `tests/unit/integrations/pm-conformance.test.ts`. Trivial providers can reuse `createFakePMProvider()` (see Trello/JIRA/Linear fixtures). + +7. **Run the conformance harness**: `npx vitest run --project unit-core tests/unit/integrations/pm-conformance.test.ts`. Behavioral contracts run against your provider automatically once `configSchema` / `discoveryCapabilities` / `lifecycle` are declared. Failures name the contract. + +8. **Provider-specific unit tests** in `tests/unit/pm//` — adapter tests (vi.mock the client), config-schema round-trip, discovery shape, wizardSpec, adapter branded IDs. + +The shared orchestration files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, `pm-wizard-common-steps.tsx`) require zero edits beyond the barrel import in step 5. The `new-provider-surface` snapshot test proves your PR does not modify shared router / worker / CLI / dashboard orchestration or central schema files. The one deliberate shared-dashboard exception is `pm-wizard-state.ts` (step 4 above). --- diff --git a/tests/unit/integrations/new-provider-surface.test.ts b/tests/unit/integrations/new-provider-surface.test.ts index 3a83dbe20..dff5f9edc 100644 --- a/tests/unit/integrations/new-provider-surface.test.ts +++ b/tests/unit/integrations/new-provider-surface.test.ts @@ -44,6 +44,11 @@ const SHARED_SURFACE_FILES = [ 'src/api/routers/pm-discovery.ts', 'web/src/components/projects/pm-providers/generator.tsx', + // Frontend PM provider barrel — new providers add one import here, + // just like the backend barrel at src/integrations/pm/index.ts. + // pm-wizard.tsx imports this barrel and never needs to change. + 'web/src/components/projects/pm-providers/index.ts', + // Shared wizard step components (plan 010/3 + plan 011/1) — real // components for every StandardStepKind. A new provider with purely // standard steps never touches these files; it declares @@ -94,7 +99,8 @@ describe('new-provider-surface (plan 009/5 task 4, spec 009 AC #10)', () => { 'required for a new provider lives in:', ' - src/integrations/pm//', ' - web/src/components/projects/pm-providers//', - ' - A single import line in src/integrations/pm/index.ts', + ' - A single import line in src/integrations/pm/index.ts (backend barrel)', + ' - A single import line in web/src/components/projects/pm-providers/index.ts (frontend barrel)', 'If you need to edit one of the shared surface files above, ', 'update this test with the justification and the new expected state.', ].join('\n'); diff --git a/tests/unit/web/pm-wizard-hooks-architecture.test.ts b/tests/unit/web/pm-wizard-hooks-architecture.test.ts new file mode 100644 index 000000000..edb2aa300 --- /dev/null +++ b/tests/unit/web/pm-wizard-hooks-architecture.test.ts @@ -0,0 +1,86 @@ +import { readFileSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); +const REPO_ROOT = resolve(__dirname, '..', '..', '..'); +const PM_WIZARD_HOOKS_PATH = resolve(REPO_ROOT, 'web/src/components/projects/pm-wizard-hooks.ts'); + +const PROVIDER_SPECIFIC_HOOK_EXPORTS = [ + 'useTrelloDiscovery', + 'useTrelloLabelCreation', + 'useTrelloCustomFieldCreation', + 'useJiraDiscovery', + 'useJiraLabelCreation', + 'useJiraCustomFieldCreation', + 'useLinearDiscovery', + 'useLinearLabelCreation', + 'useLinearCustomFieldCreation', +] as const; + +function readSharedHooks(): string { + return readFileSync(PM_WIZARD_HOOKS_PATH, 'utf8'); +} + +describe('pm-wizard-hooks architecture boundary', () => { + it('does not export provider-specific hook wrappers', () => { + const source = readSharedHooks(); + + for (const hookName of PROVIDER_SPECIFIC_HOOK_EXPORTS) { + expect( + source, + `${hookName} must live under web/src/components/projects/pm-providers//hooks.ts, not pm-wizard-hooks.ts`, + ).not.toMatch(new RegExp(`\\bexport\\s+(?:function|const)\\s+${hookName}\\b`)); + expect(source, `${hookName} must not be re-exported from pm-wizard-hooks.ts`).not.toMatch( + new RegExp(`\\bexport\\s*\\{[^}]*\\b${hookName}\\b[^}]*\\}`), + ); + } + }); + + it('does not import provider-owned modules', () => { + const source = readSharedHooks(); + + expect( + source, + 'pm-wizard-hooks.ts must stay provider-agnostic; import provider-owned hook/auth modules from provider folders instead.', + ).not.toMatch(/from ['"].*\/pm-providers\/(?:trello|jira|linear)\//); + expect( + source, + 'pm-wizard-hooks.ts must stay provider-agnostic; do not import backend provider modules directly.', + ).not.toMatch(/from ['"].*\/src\/integrations\/pm\/(?:trello|jira|linear)\//); + }); + + it('does not contain provider-owned verification or save dispatch maps', () => { + const source = readSharedHooks(); + + expect( + source, + 'SAVE_CONFIGS belongs in provider metadata, not pm-wizard-hooks.ts', + ).not.toContain('SAVE_CONFIGS'); + expect( + source, + 'Verification/save auth must be metadata-driven; do not branch on state.provider in pm-wizard-hooks.ts.', + ).not.toMatch(/\bstate\.provider\s*={2,3}\s*['"](?:trello|jira|linear)['"]/); + expect( + source, + 'Provider-specific verified-as display formatting belongs on ProviderWizardDefinition.formatVerificationDisplay.', + ).not.toMatch(/\bexport\s+function\s+formatVerificationDisplay\b/); + }); + + it('does not hard-code provider ids in shared config type declarations', () => { + const source = readSharedHooks(); + + // LabelCreationConfig and CustomFieldCreationConfig must accept any provider id + // so that new providers can use the shared factories without editing this file. + expect( + source, + "LabelCreationConfig.providerId must be 'string', not a literal union — new providers must not need to edit pm-wizard-hooks.ts", + ).not.toMatch(/providerId:\s*'(?:trello|jira|linear)'\s*\|/); + expect( + source, + "runPerLabelCreations opts.providerId must be 'string', not a literal union", + ).not.toMatch(/providerId:\s*'(?:trello|jira|linear)'\s*\|/); + }); +}); diff --git a/tests/unit/web/pm-wizard-hooks.test.ts b/tests/unit/web/pm-wizard-hooks.test.ts index 019ce45cb..56eca3952 100644 --- a/tests/unit/web/pm-wizard-hooks.test.ts +++ b/tests/unit/web/pm-wizard-hooks.test.ts @@ -1,6 +1,5 @@ /** * Unit tests for pure functions extracted in the pm-wizard-hooks refactor: - * - buildProviderAuthArg (generic auth-arg builder for all three providers) * - runPerLabelCreations (batch label creator with per-item error handling) * - buildTrelloIntegrationConfig / buildJiraIntegrationConfig (pure config builders) */ @@ -13,9 +12,7 @@ import { buildCurrentUserDiscoveryRequest, buildIntegrationUpsertInput, buildPersistedCredentialInputs, - buildProviderAuthArg, buildProviderAuthArgFromMetadata, - formatVerificationDisplay, runPerLabelCreations, } from '../../../web/src/components/projects/pm-wizard-hooks.js'; import type { WizardState } from '../../../web/src/components/projects/pm-wizard-state.js'; @@ -26,142 +23,6 @@ import { createInitialState, } from '../../../web/src/components/projects/pm-wizard-state.js'; -// ============================================================================ -// buildProviderAuthArg -// ============================================================================ - -describe('buildProviderAuthArg', () => { - function trelloState(overrides: Partial = {}): WizardState { - return { ...createInitialState(), provider: 'trello', ...overrides }; - } - function jiraState(overrides: Partial = {}): WizardState { - return { ...createInitialState(), provider: 'jira', ...overrides }; - } - function linearState(overrides: Partial = {}): WizardState { - return { ...createInitialState(), provider: 'linear', ...overrides }; - } - - // ── Edit mode — stored credentials path ────────────────────────────── - it('trello: returns { projectId } in edit mode when stored creds and no raw key', () => { - const state = trelloState({ - isEditing: true, - hasStoredCredentials: true, - trelloApiKey: '', - trelloToken: '', - }); - expect(buildProviderAuthArg(state, 'proj-1')).toEqual({ projectId: 'proj-1' }); - }); - - it('jira: returns { projectId } in edit mode when stored creds and no raw token', () => { - const state = jiraState({ - isEditing: true, - hasStoredCredentials: true, - jiraApiToken: '', - jiraEmail: '', - }); - expect(buildProviderAuthArg(state, 'proj-jira')).toEqual({ projectId: 'proj-jira' }); - }); - - it('linear: returns { projectId } in edit mode when stored creds and no raw key', () => { - const state = linearState({ - isEditing: true, - hasStoredCredentials: true, - linearApiKey: '', - }); - expect(buildProviderAuthArg(state, 'proj-lin')).toEqual({ projectId: 'proj-lin' }); - }); - - // ── Fresh setup — credentials path ────────────────────────────────── - it('trello: returns credentials when api_key and token present (fresh setup)', () => { - const state = trelloState({ trelloApiKey: 'key-abc', trelloToken: 'tok-xyz' }); - expect(buildProviderAuthArg(state, 'proj-1')).toEqual({ - credentials: { api_key: 'key-abc', token: 'tok-xyz' }, - }); - }); - - it('jira: returns credentials when email + api_token + base_url present (fresh setup)', () => { - const state = jiraState({ - jiraEmail: 'user@example.com', - jiraApiToken: 'jira-tok', - jiraBaseUrl: 'https://example.atlassian.net', - }); - expect(buildProviderAuthArg(state, 'proj-j')).toEqual({ - credentials: { - email: 'user@example.com', - api_token: 'jira-tok', - base_url: 'https://example.atlassian.net', - }, - }); - }); - - it('linear: returns credentials when api_key present (fresh setup)', () => { - const state = linearState({ linearApiKey: 'lin_abc' }); - expect(buildProviderAuthArg(state, 'proj-l')).toEqual({ - credentials: { api_key: 'lin_abc' }, - }); - }); - - // ── Edit mode — user re-typed key → use fresh credentials ─────────── - it('trello: uses fresh credentials when user re-typed api_key in edit mode', () => { - const state = trelloState({ - isEditing: true, - hasStoredCredentials: true, - trelloApiKey: 'new-key', - trelloToken: 'new-tok', - }); - expect(buildProviderAuthArg(state, 'proj-1')).toEqual({ - credentials: { api_key: 'new-key', token: 'new-tok' }, - }); - }); - - it('linear: uses fresh credentials when user re-typed api_key in edit mode', () => { - const state = linearState({ - isEditing: true, - hasStoredCredentials: true, - linearApiKey: 'lin_fresh', - }); - expect(buildProviderAuthArg(state, 'proj-l')).toEqual({ - credentials: { api_key: 'lin_fresh' }, - }); - }); - - // ── Error cases ────────────────────────────────────────────────────── - it('trello: throws when no api_key in fresh mode', () => { - const state = trelloState({ trelloToken: 'tok' }); - expect(() => buildProviderAuthArg(state, 'proj-1')).toThrow( - 'Enter both credentials before verifying', - ); - }); - - it('trello: throws when no token in fresh mode', () => { - const state = trelloState({ trelloApiKey: 'key' }); - expect(() => buildProviderAuthArg(state, 'proj-1')).toThrow( - 'Enter both credentials before verifying', - ); - }); - - it('jira: throws when no email in fresh mode', () => { - const state = jiraState({ jiraApiToken: 'tok', jiraBaseUrl: 'https://x.atlassian.net' }); - expect(() => buildProviderAuthArg(state, 'proj-j')).toThrow( - 'Enter both credentials before verifying', - ); - }); - - it('jira: throws when no api_token in fresh mode', () => { - const state = jiraState({ jiraEmail: 'u@x.com', jiraBaseUrl: 'https://x.atlassian.net' }); - expect(() => buildProviderAuthArg(state, 'proj-j')).toThrow( - 'Enter both credentials before verifying', - ); - }); - - it('linear: throws when no api_key in fresh mode', () => { - const state = linearState({ linearApiKey: '' }); - expect(() => buildProviderAuthArg(state, 'proj-l')).toThrow( - 'Enter your API key before verifying', - ); - }); -}); - // ============================================================================ // Provider-owned credential metadata // ============================================================================ @@ -358,19 +219,27 @@ describe('metadata-driven verification request', () => { it('preserves provider-specific verified-as display formatting', () => { expect( - formatVerificationDisplay('trello', { id: '1', name: 'Full Name', displayName: 'user' }), + trelloProviderWizard.formatVerificationDisplay({ + id: '1', + name: 'Full Name', + displayName: 'user', + }), ).toBe('@user (Full Name)'); expect( - formatVerificationDisplay('jira', { + jiraProviderWizard.formatVerificationDisplay({ id: '2', name: 'Jira User', displayName: 'user@example.com', }), ).toBe('Jira User (user@example.com)'); expect( - formatVerificationDisplay('linear', { id: '3', name: 'Linear User', displayName: 'lin' }), + linearProviderWizard.formatVerificationDisplay({ + id: '3', + name: 'Linear User', + displayName: 'lin', + }), ).toBe('lin'); - expect(formatVerificationDisplay('linear', { id: '4', name: 'Linear User' })).toBe( + expect(linearProviderWizard.formatVerificationDisplay({ id: '4', name: 'Linear User' })).toBe( 'Linear User', ); }); diff --git a/web/src/components/projects/pm-providers/index.ts b/web/src/components/projects/pm-providers/index.ts new file mode 100644 index 000000000..b4116bc5e --- /dev/null +++ b/web/src/components/projects/pm-providers/index.ts @@ -0,0 +1,23 @@ +/** + * Frontend PM provider barrel — side-effect imports register each provider's + * wizard definition into `providerWizardRegistry` at module load. + * + * This mirrors the backend barrel at `src/integrations/pm/index.ts`. The + * single import of this module in `pm-wizard.tsx` ensures every provider is + * registered before the wizard renders. + * + * Adding a new PM provider? Add exactly one line here: + * import './/index.js'; + * + * Shared orchestration files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, + * `pm-wizard-common-steps.tsx`) need zero edits after this import. + * The one shared dashboard file that still requires manual edits is + * `pm-wizard-state.ts` — new providers must add their credential fields + * to `WizardState`, the corresponding action types to `WizardAction`, and + * `buildEditState` handling for their config shape. See step 4 of + * "Adding a new PM provider" in src/integrations/README.md. + */ + +import './trello/index.js'; +import './jira/index.js'; +import './linear/index.js'; diff --git a/web/src/components/projects/pm-providers/jira/auth.ts b/web/src/components/projects/pm-providers/jira/auth.ts new file mode 100644 index 000000000..7767419ec --- /dev/null +++ b/web/src/components/projects/pm-providers/jira/auth.ts @@ -0,0 +1,16 @@ +import type { ProviderAuthMetadata, ProviderCredentialPersistenceMapping } from '../types.js'; + +export const jiraAuthMetadata: ProviderAuthMetadata = { + rawCredentials: [ + { role: 'email', stateField: 'jiraEmail' }, + { role: 'api_token', stateField: 'jiraApiToken' }, + { role: 'base_url', stateField: 'jiraBaseUrl' }, + ], + storedCredentials: { fallbackWhenStateFieldEmpty: 'jiraEmail' }, + missingCredentialsMessage: 'Enter both credentials before verifying', +}; + +export const jiraCredentialPersistence: readonly ProviderCredentialPersistenceMapping[] = [ + { envVarKey: 'JIRA_EMAIL', stateField: 'jiraEmail', label: 'JIRA Email' }, + { envVarKey: 'JIRA_API_TOKEN', stateField: 'jiraApiToken', label: 'JIRA API Token' }, +]; diff --git a/web/src/components/projects/pm-providers/jira/hooks.ts b/web/src/components/projects/pm-providers/jira/hooks.ts index 037712e4d..6958af3df 100644 --- a/web/src/components/projects/pm-providers/jira/hooks.ts +++ b/web/src/components/projects/pm-providers/jira/hooks.ts @@ -4,6 +4,7 @@ import { useEffect } from 'react'; import { trpcClient } from '@/lib/trpc.js'; import { useProviderCustomFieldCreation } from '../../pm-wizard-hooks.js'; import type { WizardAction, WizardState } from '../../pm-wizard-state.js'; +import { jiraAuthMetadata } from './auth.js'; // ============================================================================ // JIRA Discovery @@ -122,6 +123,7 @@ export function useJiraCustomFieldCreation( const inner = useProviderCustomFieldCreation( { providerId: 'jira', + auth: jiraAuthMetadata, // JIRA fields are global; containerId is sent as-is for uniform shape getContainerId: (s) => s.jiraProjectKey || 'global', addCustomField: (f) => ({ diff --git a/web/src/components/projects/pm-providers/jira/wizard.ts b/web/src/components/projects/pm-providers/jira/wizard.ts index 5dc94ebae..ba978be39 100644 --- a/web/src/components/projects/pm-providers/jira/wizard.ts +++ b/web/src/components/projects/pm-providers/jira/wizard.ts @@ -27,6 +27,7 @@ import { CustomFieldMappingStep } from '../steps/custom-field-mapping.js'; import { LabelMappingStep } from '../steps/label-mapping.js'; import { StatusMappingStep } from '../steps/status-mapping.js'; import type { ProviderWizardDefinition, ProviderWizardStepProps } from '../types.js'; +import { jiraAuthMetadata, jiraCredentialPersistence } from './auth.js'; import { useJiraCustomFieldCreation, useJiraDiscovery } from './hooks.js'; import { IssueTypeMappingStep } from './issue-type-step.js'; import { JiraWebhookAdapter } from './webhook-step.js'; @@ -248,19 +249,9 @@ function JiraIssueTypeAdapter({ export const jiraProviderWizard: ProviderWizardDefinition = { id: 'jira', label: 'JIRA', - auth: { - rawCredentials: [ - { role: 'email', stateField: 'jiraEmail' }, - { role: 'api_token', stateField: 'jiraApiToken' }, - { role: 'base_url', stateField: 'jiraBaseUrl' }, - ], - storedCredentials: { fallbackWhenStateFieldEmpty: 'jiraEmail' }, - missingCredentialsMessage: 'Enter both credentials before verifying', - }, - credentialPersistence: [ - { envVarKey: 'JIRA_EMAIL', stateField: 'jiraEmail', label: 'JIRA Email' }, - { envVarKey: 'JIRA_API_TOKEN', stateField: 'jiraApiToken', label: 'JIRA API Token' }, - ], + auth: jiraAuthMetadata, + formatVerificationDisplay: (me) => (me.displayName ? `${me.name} (${me.displayName})` : me.name), + credentialPersistence: jiraCredentialPersistence, steps: [ { diff --git a/web/src/components/projects/pm-providers/linear/auth.ts b/web/src/components/projects/pm-providers/linear/auth.ts new file mode 100644 index 000000000..5524c0ae1 --- /dev/null +++ b/web/src/components/projects/pm-providers/linear/auth.ts @@ -0,0 +1,11 @@ +import type { ProviderAuthMetadata, ProviderCredentialPersistenceMapping } from '../types.js'; + +export const linearAuthMetadata: ProviderAuthMetadata = { + rawCredentials: [{ role: 'api_key', stateField: 'linearApiKey' }], + storedCredentials: { fallbackWhenStateFieldEmpty: 'linearApiKey' }, + missingCredentialsMessage: 'Enter your API key before verifying', +}; + +export const linearCredentialPersistence: readonly ProviderCredentialPersistenceMapping[] = [ + { envVarKey: 'LINEAR_API_KEY', stateField: 'linearApiKey', label: 'Linear API Key' }, +]; diff --git a/web/src/components/projects/pm-providers/linear/hooks.ts b/web/src/components/projects/pm-providers/linear/hooks.ts index 5644fcd3d..033d03c0b 100644 --- a/web/src/components/projects/pm-providers/linear/hooks.ts +++ b/web/src/components/projects/pm-providers/linear/hooks.ts @@ -10,6 +10,7 @@ import type { WizardAction, WizardState, } from '../../pm-wizard-state.js'; +import { linearAuthMetadata } from './auth.js'; // ============================================================================ // Linear Discovery @@ -162,6 +163,7 @@ export function useLinearLabelCreation( return useProviderLabelCreation( { providerId: 'linear', + auth: linearAuthMetadata, getContainerId: (s) => s.linearTeamId, containerError: 'Team must be selected before creating a label', addLabel: (label) => ({ type: 'ADD_LINEAR_TEAM_LABEL', label }), diff --git a/web/src/components/projects/pm-providers/linear/wizard.ts b/web/src/components/projects/pm-providers/linear/wizard.ts index 5ba51a418..76eaa8621 100644 --- a/web/src/components/projects/pm-providers/linear/wizard.ts +++ b/web/src/components/projects/pm-providers/linear/wizard.ts @@ -31,6 +31,7 @@ import { LabelMappingStep } from '../steps/label-mapping.js'; import { ProjectScopeStep } from '../steps/project-scope.js'; import { StatusMappingStep } from '../steps/status-mapping.js'; import type { ProviderWizardDefinition, ProviderWizardStepProps } from '../types.js'; +import { linearAuthMetadata, linearCredentialPersistence } from './auth.js'; import { useLinearDiscovery, useLinearLabelCreation } from './hooks.js'; import { LinearWebhookAdapter } from './webhook-step.js'; @@ -231,14 +232,9 @@ function LinearProjectScopeAdapter({ providerHooks }: ProviderWizardStepProps): export const linearProviderWizard: ProviderWizardDefinition = { id: 'linear', label: 'Linear', - auth: { - rawCredentials: [{ role: 'api_key', stateField: 'linearApiKey' }], - storedCredentials: { fallbackWhenStateFieldEmpty: 'linearApiKey' }, - missingCredentialsMessage: 'Enter your API key before verifying', - }, - credentialPersistence: [ - { envVarKey: 'LINEAR_API_KEY', stateField: 'linearApiKey', label: 'Linear API Key' }, - ], + auth: linearAuthMetadata, + formatVerificationDisplay: (me) => me.displayName || me.name, + credentialPersistence: linearCredentialPersistence, steps: [ { diff --git a/web/src/components/projects/pm-providers/trello/auth.ts b/web/src/components/projects/pm-providers/trello/auth.ts new file mode 100644 index 000000000..302e29649 --- /dev/null +++ b/web/src/components/projects/pm-providers/trello/auth.ts @@ -0,0 +1,15 @@ +import type { ProviderAuthMetadata, ProviderCredentialPersistenceMapping } from '../types.js'; + +export const trelloAuthMetadata: ProviderAuthMetadata = { + rawCredentials: [ + { role: 'api_key', stateField: 'trelloApiKey' }, + { role: 'token', stateField: 'trelloToken' }, + ], + storedCredentials: { fallbackWhenStateFieldEmpty: 'trelloApiKey' }, + missingCredentialsMessage: 'Enter both credentials before verifying', +}; + +export const trelloCredentialPersistence: readonly ProviderCredentialPersistenceMapping[] = [ + { envVarKey: 'TRELLO_API_KEY', stateField: 'trelloApiKey', label: 'Trello API Key' }, + { envVarKey: 'TRELLO_TOKEN', stateField: 'trelloToken', label: 'Trello Token' }, +]; diff --git a/web/src/components/projects/pm-providers/trello/hooks.ts b/web/src/components/projects/pm-providers/trello/hooks.ts index 803bf333d..b708cc7c3 100644 --- a/web/src/components/projects/pm-providers/trello/hooks.ts +++ b/web/src/components/projects/pm-providers/trello/hooks.ts @@ -4,6 +4,7 @@ import { useEffect } from 'react'; import { trpcClient } from '@/lib/trpc.js'; import { useProviderCustomFieldCreation, useProviderLabelCreation } from '../../pm-wizard-hooks.js'; import type { WizardAction, WizardState } from '../../pm-wizard-state.js'; +import { trelloAuthMetadata } from './auth.js'; // ============================================================================ // Trello Discovery @@ -119,6 +120,7 @@ export function useTrelloLabelCreation( return useProviderLabelCreation( { providerId: 'trello', + auth: trelloAuthMetadata, getContainerId: (s) => s.trelloBoardId, containerError: 'Board must be selected before creating a label', addLabel: (label) => ({ type: 'ADD_TRELLO_BOARD_LABEL', label }), @@ -142,6 +144,7 @@ export function useTrelloCustomFieldCreation( return useProviderCustomFieldCreation( { providerId: 'trello', + auth: trelloAuthMetadata, getContainerId: (s) => s.trelloBoardId, containerError: 'Board must be selected before creating a custom field', addCustomField: (f) => ({ type: 'ADD_TRELLO_BOARD_CUSTOM_FIELD', customField: f }), diff --git a/web/src/components/projects/pm-providers/trello/wizard.ts b/web/src/components/projects/pm-providers/trello/wizard.ts index 51919e52a..f250e491b 100644 --- a/web/src/components/projects/pm-providers/trello/wizard.ts +++ b/web/src/components/projects/pm-providers/trello/wizard.ts @@ -27,6 +27,7 @@ import { CustomFieldMappingStep } from '../steps/custom-field-mapping.js'; import { LabelMappingStep } from '../steps/label-mapping.js'; import { StatusMappingStep } from '../steps/status-mapping.js'; import type { ProviderWizardDefinition, ProviderWizardStepProps } from '../types.js'; +import { trelloAuthMetadata, trelloCredentialPersistence } from './auth.js'; import { useTrelloCustomFieldCreation, useTrelloDiscovery, @@ -247,18 +248,9 @@ function TrelloCustomFieldMappingAdapter({ export const trelloProviderWizard: ProviderWizardDefinition = { id: 'trello', label: 'Trello', - auth: { - rawCredentials: [ - { role: 'api_key', stateField: 'trelloApiKey' }, - { role: 'token', stateField: 'trelloToken' }, - ], - storedCredentials: { fallbackWhenStateFieldEmpty: 'trelloApiKey' }, - missingCredentialsMessage: 'Enter both credentials before verifying', - }, - credentialPersistence: [ - { envVarKey: 'TRELLO_API_KEY', stateField: 'trelloApiKey', label: 'Trello API Key' }, - { envVarKey: 'TRELLO_TOKEN', stateField: 'trelloToken', label: 'Trello Token' }, - ], + auth: trelloAuthMetadata, + formatVerificationDisplay: (me) => (me.displayName ? `@${me.displayName} (${me.name})` : me.name), + credentialPersistence: trelloCredentialPersistence, // Each step mirrors `trelloManifest.wizardSpec.steps` by id. steps: [ diff --git a/web/src/components/projects/pm-providers/types.ts b/web/src/components/projects/pm-providers/types.ts index 506625be3..879009553 100644 --- a/web/src/components/projects/pm-providers/types.ts +++ b/web/src/components/projects/pm-providers/types.ts @@ -95,6 +95,12 @@ export interface ProviderWizardDefinition { readonly steps: readonly ProviderWizardStep[]; /** Provider-owned auth contract for raw credentials and stored fallback. */ readonly auth: ProviderAuthMetadata; + /** Formats the provider's normalized current-user discovery response. */ + readonly formatVerificationDisplay: (me: { + readonly id: string; + readonly name: string; + readonly displayName?: string; + }) => string; /** Normal provider credentials saved to project_credentials. */ readonly credentialPersistence: readonly ProviderCredentialPersistenceMapping[]; /** diff --git a/web/src/components/projects/pm-wizard-hooks.ts b/web/src/components/projects/pm-wizard-hooks.ts index d73fd661c..b8ccc942c 100644 --- a/web/src/components/projects/pm-wizard-hooks.ts +++ b/web/src/components/projects/pm-wizard-hooks.ts @@ -3,7 +3,7 @@ * Each hook encapsulates one concern to keep the main orchestrator thin. * * Generic hooks introduced in spec 013 refactor: - * - buildProviderAuthArg — single auth-arg builder for all three providers + * - buildProviderAuthArgFromMetadata — auth-arg builder driven by provider metadata * - useProviderLabelCreation— parameterized label-creation hook (replaces 2 copies) * - useProviderCustomFieldCreation — parameterized CF hook (replaces 2 copies) * - useSaveMutation — data-driven, no provider branching @@ -15,50 +15,21 @@ import { trpc, trpcClient } from '@/lib/trpc.js'; import { getCredentialRoles } from '../../../../src/config/integrationRoles.js'; import type { ProviderAuthMetadata, ProviderWizardDefinition } from './pm-providers/types.js'; import type { WizardAction, WizardState } from './pm-wizard-state.js'; -import { shouldUseStoredCredentials } from './pm-wizard-state.js'; - -// ============================================================================ -// Auth-arg builder — shared across all mutations -// ============================================================================ /** - * Build the `{ projectId }` or `{ credentials: ... }` portion of a tRPC - * request for any provider. Returns the stored-creds path when the user is - * editing an existing integration without re-typing their key. - * - * Extracted so every per-provider mutation stays below the cognitive- - * complexity threshold and a single place enforces the invariant. + * Returns true when the provider's credentials form fields are all non-empty. + * Driven entirely by the provider's `auth.rawCredentials` metadata so no + * shared file needs to change when a new provider declares its credential + * fields. Used to enable the "Verify Connection" button. */ -export function buildProviderAuthArg( +export function areCredentialsReadyFromMetadata( state: WizardState, - projectId: string, -): { projectId: string } | { credentials: Record } { - if (shouldUseStoredCredentials(state)) { - return { projectId }; - } - if (state.provider === 'trello') { - if (!state.trelloApiKey || !state.trelloToken) { - throw new Error('Enter both credentials before verifying'); - } - return { credentials: { api_key: state.trelloApiKey, token: state.trelloToken } }; - } - if (state.provider === 'linear') { - if (!state.linearApiKey) { - throw new Error('Enter your API key before verifying'); - } - return { credentials: { api_key: state.linearApiKey } }; - } - // jira - if (!state.jiraEmail || !state.jiraApiToken) { - throw new Error('Enter both credentials before verifying'); - } - return { - credentials: { - email: state.jiraEmail, - api_token: state.jiraApiToken, - base_url: state.jiraBaseUrl, - }, - }; + auth: ProviderAuthMetadata, +): boolean { + return auth.rawCredentials.every((cred) => { + const value = state[cred.stateField]; + return typeof value === 'string' && !!value; + }); } export function buildProviderAuthArgFromMetadata( @@ -66,8 +37,13 @@ export function buildProviderAuthArgFromMetadata( projectId: string, metadata: ProviderAuthMetadata, ): { projectId: string } | { credentials: Record } { + // In edit mode with stored credentials, pass { projectId } when the primary + // credential field is still empty (user hasn't re-typed their key). The + // fallbackWhenStateFieldEmpty field from the provider's auth metadata tells + // us which state field to check — no provider-specific branching needed. if ( - shouldUseStoredCredentials(state) && + state.isEditing && + state.hasStoredCredentials && !state[metadata.storedCredentials.fallbackWhenStateFieldEmpty] ) { return { projectId }; @@ -97,7 +73,7 @@ export function buildProviderAuthArgFromMetadata( */ export async function runPerLabelCreations(opts: { labelsToCreate: Array<{ slot: string; name: string; color?: string }>; - providerId: 'trello' | 'linear'; + providerId: string; containerId: string; authArg: { projectId: string } | { credentials: Record }; }): Promise<{ @@ -128,7 +104,9 @@ export async function runPerLabelCreations(opts: { // ============================================================================ interface LabelCreationConfig { - providerId: 'trello' | 'linear'; + providerId: string; + /** Provider-owned auth contract for raw credentials and stored fallback */ + auth: ProviderAuthMetadata; /** Returns the container ID (board / team) from state */ getContainerId: (state: WizardState) => string; /** Error when container not yet selected */ @@ -149,7 +127,7 @@ function useProviderLabelCreation( mutationFn: (vars: { name: string; color?: string; slot: string }) => { const containerId = config.getContainerId(state); if (!containerId) throw new Error(config.containerError); - const authArg = buildProviderAuthArg(state, projectId); + const authArg = buildProviderAuthArgFromMetadata(state, projectId, config.auth); return trpcClient.pm.discovery.createLabel.mutate({ providerId: config.providerId, containerId, @@ -172,7 +150,7 @@ function useProviderLabelCreation( mutationFn: async (labelsToCreate: Array<{ slot: string; name: string; color?: string }>) => { const containerId = config.getContainerId(state); if (!containerId) throw new Error(config.containerError); - const authArg = buildProviderAuthArg(state, projectId); + const authArg = buildProviderAuthArgFromMetadata(state, projectId, config.auth); return runPerLabelCreations({ labelsToCreate, providerId: config.providerId, @@ -209,7 +187,9 @@ function useProviderLabelCreation( // ============================================================================ interface CustomFieldCreationConfig { - providerId: 'trello' | 'jira'; + providerId: string; + /** Provider-owned auth contract for raw credentials and stored fallback */ + auth: ProviderAuthMetadata; /** Returns the container ID from state (boardId / projectKey) */ getContainerId: (state: WizardState) => string; /** Error thrown when container not yet selected (required for Trello; omit for global providers like JIRA) */ @@ -232,7 +212,7 @@ function useProviderCustomFieldCreation( mutationFn: ({ name }: { name: string }) => { const containerId = config.getContainerId(state); if (!containerId && config.containerError) throw new Error(config.containerError); - const authArg = buildProviderAuthArg(state, projectId); + const authArg = buildProviderAuthArgFromMetadata(state, projectId, config.auth); return trpcClient.pm.discovery.createCustomField.mutate({ providerId: config.providerId, containerId: containerId || 'global', @@ -285,23 +265,6 @@ export function buildCurrentUserDiscoveryRequest( }; } -export function formatVerificationDisplay( - provider: string, - me: { id: string; name: string; displayName?: string }, -): string { - // Per-provider display formatting mirrors the pre-009/5 UX: - // Trello: "@{username} ({fullName})" — displayName is username - // JIRA: "{displayName} ({email})" — displayName is email - // Linear: "{displayName || name}" — displayName is the preferred handle - if (provider === 'trello') { - return me.displayName ? `@${me.displayName} (${me.name})` : me.name; - } - if (provider === 'jira') { - return me.displayName ? `${me.name} (${me.displayName})` : me.name; - } - return me.displayName || me.name; -} - export function useVerification( state: WizardState, dispatch: Dispatch, @@ -331,7 +294,7 @@ export function useVerification( if (provider !== state.provider) return; dispatch({ type: 'SET_VERIFICATION', - result: { provider, display: formatVerificationDisplay(provider, me) }, + result: { provider, display: manifestDef.formatVerificationDisplay(me) }, }); advanceToStep(3); }, diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index af21a6185..dc42a3662 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -49,7 +49,20 @@ export interface LinearTeamDetails { labels: Array<{ id: string; name: string; color: string }>; } -export type Provider = 'trello' | 'jira' | 'linear'; +/** + * Provider identifier — an open string so new providers registered via the + * frontend barrel (`web/src/components/projects/pm-providers/index.ts`) can + * be dispatched without adding to a closed union here. + * + * Note: adding a new PM provider still requires updating `WizardState` with + * the provider's credential fields (e.g. `asanaApiKey: string`), the reducer + * with the corresponding action types, and `buildEditState` with the new + * provider's config-shape handling. The credential-readiness path + * (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the + * mutation auth path (`buildProviderAuthArgFromMetadata`) are metadata-driven + * and do NOT require changes. + */ +export type Provider = string; export interface WizardState { provider: Provider; diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index 453e5f73f..353453f6c 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -3,24 +3,25 @@ import { CheckCircle, Globe, Loader2, XCircle } from 'lucide-react'; import { useEffect, useReducer, useRef, useState } from 'react'; import { Label } from '@/components/ui/label.js'; import { trpc } from '@/lib/trpc.js'; -// Side-effect imports register every PM provider's frontend wizard into -// the provider registry. With Linear migrated (006/4), every PM provider -// now renders via the manifest shell. -import './pm-providers/trello/index.js'; -import './pm-providers/jira/index.js'; -import './pm-providers/linear/index.js'; +// Single barrel import registers every PM provider's frontend wizard into the +// provider registry. New providers add one import to pm-providers/index.ts — +// this file never needs to change for a new provider. +import './pm-providers/index.js'; import { ManifestProviderWizardSection } from './pm-providers/manifest-section.js'; -import { getProviderWizard } from './pm-providers/registry.js'; +import { getProviderWizard, listProviderWizards } from './pm-providers/registry.js'; import type { ProviderWizardDefinition } from './pm-providers/types.js'; import { SaveStep } from './pm-wizard-common-steps.js'; -import { useSaveMutation, useVerification } from './pm-wizard-hooks.js'; +import { + areCredentialsReadyFromMetadata, + useSaveMutation, + useVerification, +} from './pm-wizard-hooks.js'; // Plan 011/5: the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` // files were deleted; all three providers now render exclusively through // the manifest path (see `./pm-providers//wizard.ts`). // Plan 012/4: `WebhookStep` + `LinearWebhookInfoPanel` + supporting hooks // deleted; every provider owns its webhook UX via the manifest path. import { - areCredentialsReady, buildEditState, createInitialState, isStep1Complete, @@ -38,18 +39,9 @@ import { WizardStep } from './wizard-shared.js'; // (manifestDef.steps[i].title). Only step 1 (provider picker) and the // legacy Webhook + Save slots have fixed titles; rendered inline. -const PROVIDER_LABELS: Record<'trello' | 'jira' | 'linear', string> = { - trello: 'Trello', - jira: 'JIRA', - linear: 'Linear', -}; - -function confirmProviderSwitch( - from: 'trello' | 'jira' | 'linear', - to: 'trello' | 'jira' | 'linear', -): boolean { +function confirmProviderSwitch(fromLabel: string, toLabel: string): boolean { return window.confirm( - `Switch PM provider from ${PROVIDER_LABELS[from]} to ${PROVIDER_LABELS[to]}?\n\nYou'll need to re-enter credentials and re-map fields for ${PROVIDER_LABELS[to]}. The old provider's credentials will be deleted when you save.`, + `Switch PM provider from ${fromLabel} to ${toLabel}?\n\nYou'll need to re-enter credentials and re-map fields for ${toLabel}. The old provider's credentials will be deleted when you save.`, ); } @@ -253,7 +245,9 @@ export function PMWizard({ // ---- Step status ---- - const credsReady = areCredentialsReady(state); + // Metadata-driven: reads rawCredentials from the provider's auth spec so + // no shared file needs to change when a new provider is added. + const credsReady = areCredentialsReadyFromMetadata(state, manifestDef.auth); function getStatus( stepNum: number, @@ -283,23 +277,26 @@ export function PMWizard({
- {(['trello', 'jira', 'linear'] as const).map((p) => ( + {listProviderWizards().map((wizard) => ( ))}
From 36c71df7cc0b88e89d1967a497b99d103113428f Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 12:49:22 +0200 Subject: [PATCH 02/15] test(cli): isolate dashboard org env in config tests (#1315) Co-authored-by: Cascade Bot --- tests/unit/cli/dashboard/config.test.ts | 71 ++++++++++++++++++++++++- 1 file changed, 69 insertions(+), 2 deletions(-) diff --git a/tests/unit/cli/dashboard/config.test.ts b/tests/unit/cli/dashboard/config.test.ts index ea6d841bc..754b15b10 100644 --- a/tests/unit/cli/dashboard/config.test.ts +++ b/tests/unit/cli/dashboard/config.test.ts @@ -21,11 +21,21 @@ import { describe('config', () => { const originalEnv = process.env; + const DASHBOARD_CONFIG_ENV_KEYS = [ + 'CASCADE_SERVER_URL', + 'CASCADE_SESSION_TOKEN', + 'CASCADE_ORG_ID', + ] as const; + + function clearDashboardConfigEnv() { + for (const key of DASHBOARD_CONFIG_ENV_KEYS) { + delete process.env[key]; + } + } beforeEach(() => { process.env = { ...originalEnv }; - delete process.env.CASCADE_SERVER_URL; - delete process.env.CASCADE_SESSION_TOKEN; + clearDashboardConfigEnv(); }); afterEach(() => { @@ -48,6 +58,22 @@ describe('config', () => { expect(existsSync).not.toHaveBeenCalled(); }); + it('returns env orgId when env config is complete', () => { + process.env.CASCADE_SERVER_URL = 'http://env-server:3000'; + process.env.CASCADE_SESSION_TOKEN = 'env-token'; + process.env.CASCADE_ORG_ID = 'env-org'; + + const config = loadConfig(); + + expect(config).toEqual({ + serverUrl: 'http://env-server:3000', + sessionToken: 'env-token', + cookieName: SESSION_COOKIE_NAME, + orgId: 'env-org', + }); + expect(existsSync).not.toHaveBeenCalled(); + }); + it('returns null when config file does not exist', () => { vi.mocked(existsSync).mockReturnValue(false); @@ -71,6 +97,26 @@ describe('config', () => { expect(readFileSync).toHaveBeenCalledWith(expect.stringContaining('cli.json'), 'utf-8'); }); + it('reads stored orgId from file when no env override is set', () => { + vi.mocked(existsSync).mockReturnValue(true); + vi.mocked(readFileSync).mockReturnValue( + JSON.stringify({ + serverUrl: 'http://localhost:3000', + sessionToken: 'file-token', + orgId: 'file-org', + }), + ); + + const config = loadConfig(); + + expect(config).toEqual({ + serverUrl: 'http://localhost:3000', + sessionToken: 'file-token', + cookieName: SESSION_COOKIE_NAME, + orgId: 'file-org', + }); + }); + it('returns null when file has incomplete config', () => { vi.mocked(existsSync).mockReturnValue(true); vi.mocked(readFileSync).mockReturnValue(JSON.stringify({ serverUrl: 'http://x' })); @@ -119,6 +165,27 @@ describe('config', () => { }); }); + it('env orgId overrides stored orgId while preserving file server and token', () => { + process.env.CASCADE_ORG_ID = 'env-org'; + vi.mocked(existsSync).mockReturnValue(true); + vi.mocked(readFileSync).mockReturnValue( + JSON.stringify({ + serverUrl: 'http://file:3000', + sessionToken: 'file-token', + orgId: 'file-org', + }), + ); + + const config = loadConfig(); + + expect(config).toEqual({ + serverUrl: 'http://file:3000', + sessionToken: 'file-token', + cookieName: SESSION_COOKIE_NAME, + orgId: 'env-org', + }); + }); + it('loads custom cookie name from file', () => { vi.mocked(existsSync).mockReturnValue(true); vi.mocked(readFileSync).mockReturnValue( From 28114376cdac812fe918e019d83d9ae680e695bb Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 13:08:20 +0200 Subject: [PATCH 03/15] feat(web): drive PM provider picker from registry (#1316) Co-authored-by: Cascade Bot --- .../web/pm-wizard-generic-renderer.test.ts | 86 ++++++++----- .../projects/pm-providers/registry.ts | 6 +- web/src/components/projects/pm-wizard.tsx | 119 ++++++++++-------- 3 files changed, 122 insertions(+), 89 deletions(-) diff --git a/tests/unit/web/pm-wizard-generic-renderer.test.ts b/tests/unit/web/pm-wizard-generic-renderer.test.ts index 730638d0b..7409e0879 100644 --- a/tests/unit/web/pm-wizard-generic-renderer.test.ts +++ b/tests/unit/web/pm-wizard-generic-renderer.test.ts @@ -1,42 +1,41 @@ -/** - * Unit tests for the tiny registry-check helper that sits in front of the - * per-provider branches in `pm-wizard.tsx`. The helper returns the - * matching step's React element when the provider is registered, or - * `null` when it isn't — so the caller can use `??` to fall back to the - * legacy branch chain. - * - * The goal here is just to prove the seam works; integration-level SSR - * tests for the full wizard come with each provider migration (006/2–4). - */ - import { createElement } from 'react'; import { renderToStaticMarkup } from 'react-dom/server'; -import { beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { _resetProviderWizardRegistryForTesting, registerProviderWizard, } from '../../../web/src/components/projects/pm-providers/registry.js'; -import { renderManifestStep } from '../../../web/src/components/projects/pm-providers/render.js'; import type { ProviderWizardDefinition } from '../../../web/src/components/projects/pm-providers/types.js'; +import type { + buildProviderSwitchConfirmationMessage as BuildConfirmationMessage, + ProviderPicker as ProviderPickerComponent, +} from '../../../web/src/components/projects/pm-wizard.js'; +import { createInitialState } from '../../../web/src/components/projects/pm-wizard-state.js'; + +let ProviderPicker: typeof ProviderPickerComponent; +let buildProviderSwitchConfirmationMessage: typeof BuildConfirmationMessage; function StubStep({ state }: { state: { provider: string } }) { - return createElement('div', { 'data-testid': 'stub' }, `stub-${state.provider}`); + return createElement( + 'div', + { 'data-testid': `${state.provider}-stub-step` }, + `stub-${state.provider}`, + ); } -function makeStubWizard(id: string): ProviderWizardDefinition { +function makeStubWizard(id: string, label: string): ProviderWizardDefinition { return { id, - label: id, + label, steps: [ { id: 'credentials', title: 'Credentials', Component: StubStep, isComplete: () => true }, - { id: 'container', title: 'Container', Component: StubStep, isComplete: () => true }, - { id: 'fields', title: 'Field mappings', Component: StubStep, isComplete: () => true }, ], auth: { rawCredentials: [{ role: 'api_key', stateField: 'linearApiKey' }], storedCredentials: { fallbackWhenStateFieldEmpty: 'linearApiKey' }, missingCredentialsMessage: 'Missing credentials', }, + formatVerificationDisplay: (me) => me.displayName ?? me.name, credentialPersistence: [ { envVarKey: 'STUB_API_KEY', stateField: 'linearApiKey', label: 'Stub API Key' }, ], @@ -45,27 +44,48 @@ function makeStubWizard(id: string): ProviderWizardDefinition { }; } -describe('renderManifestStep', () => { +function renderProviderPicker() { + return renderToStaticMarkup( + createElement(ProviderPicker, { + state: createInitialState(), + dispatch: vi.fn(), + advanceToStep: vi.fn(), + }), + ); +} + +describe('PMWizard provider picker', () => { + beforeAll(async () => { + ({ ProviderPicker, buildProviderSwitchConfirmationMessage } = await import( + '../../../web/src/components/projects/pm-wizard.js' + )); + }); + beforeEach(() => { _resetProviderWizardRegistryForTesting(); + registerProviderWizard(makeStubWizard('trello', 'Trello')); + }); + + afterEach(() => { + _resetProviderWizardRegistryForTesting(); }); - it('renders the manifest step component when the provider is registered', () => { - registerProviderWizard(makeStubWizard('alpha')); - const element = renderManifestStep('alpha', 0, { provider: 'alpha' } as never, () => {}); - expect(element).not.toBeNull(); - const html = renderToStaticMarkup(element as React.ReactElement); - expect(html).toContain('data-testid="stub"'); - expect(html).toContain('stub-alpha'); + afterAll(() => { + _resetProviderWizardRegistryForTesting(); + }); + + it('renders every provider returned by the provider wizard registry', () => { + registerProviderWizard(makeStubWizard('acme', 'Acme PM')); + + const html = renderProviderPicker(); + + expect(html).toContain('>Trello'); + expect(html).toContain('>Acme PM'); }); - it('returns null when the provider is not registered (caller falls back to legacy)', () => { - const element = renderManifestStep( - 'unregistered', - 0, - { provider: 'unregistered' } as never, - () => {}, + it('builds provider switch confirmation copy from provider definition labels', () => { + expect(buildProviderSwitchConfirmationMessage('Trello', 'Acme PM')).toBe( + "Switch PM provider from Trello to Acme PM?\n\nYou'll need to re-enter credentials and re-map fields for Acme PM. The old provider's credentials will be deleted when you save.", ); - expect(element).toBeNull(); }); }); diff --git a/web/src/components/projects/pm-providers/registry.ts b/web/src/components/projects/pm-providers/registry.ts index e352371e7..8bced2fab 100644 --- a/web/src/components/projects/pm-providers/registry.ts +++ b/web/src/components/projects/pm-providers/registry.ts @@ -3,9 +3,9 @@ * * Providers register their wizard definition at module-load time by * calling `registerProviderWizard(def)` from the provider's frontend - * `index.ts`. The generic wizard renderer (`pm-wizard.tsx`) looks up the - * current provider here and falls back to the legacy per-provider - * branches when `getProviderWizard(id)` returns null. + * `index.ts`. The generic wizard renderer (`pm-wizard.tsx`) lists provider + * definitions from this registry for the picker and looks up the active + * provider here before rendering manifest-driven steps. */ import type { ProviderWizardDefinition } from './types.js'; diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index 353453f6c..f8105cbf6 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -39,9 +39,51 @@ import { WizardStep } from './wizard-shared.js'; // (manifestDef.steps[i].title). Only step 1 (provider picker) and the // legacy Webhook + Save slots have fixed titles; rendered inline. +export function buildProviderSwitchConfirmationMessage(fromLabel: string, toLabel: string): string { + return `Switch PM provider from ${fromLabel} to ${toLabel}?\n\nYou'll need to re-enter credentials and re-map fields for ${toLabel}. The old provider's credentials will be deleted when you save.`; +} + function confirmProviderSwitch(fromLabel: string, toLabel: string): boolean { - return window.confirm( - `Switch PM provider from ${fromLabel} to ${toLabel}?\n\nYou'll need to re-enter credentials and re-map fields for ${toLabel}. The old provider's credentials will be deleted when you save.`, + return window.confirm(buildProviderSwitchConfirmationMessage(fromLabel, toLabel)); +} + +export function ProviderPicker({ + state, + dispatch, + advanceToStep, +}: { + readonly state: WizardState; + readonly dispatch: React.Dispatch; + readonly advanceToStep: (step: number) => void; +}) { + return ( +
+ +
+ {listProviderWizards().map((wizard) => ( + + ))} +
+
); } @@ -221,10 +263,9 @@ export function PMWizard({ // ---- Custom hooks ---- - // Is there a manifest-registered wizard for the active provider? If so, - // ManifestProviderWizardSection drives the rendering (and runs the - // provider's useProviderHooks internally). Unregistered providers fall - // through to the legacy per-provider branches. + // Every selectable provider must be registered in the frontend wizard + // registry. ManifestProviderWizardSection drives all provider-owned steps + // and runs the provider's useProviderHooks internally. const manifestDef = getProviderWizard(state.provider); if (!manifestDef) { throw new Error(`No PM provider wizard registered for ${state.provider}`); @@ -274,33 +315,7 @@ export function PMWizard({ isOpen={openSteps.has(1)} onToggle={() => toggleStep(1)} > -
- -
- {listProviderWizards().map((wizard) => ( - - ))} -
-
+ {/* @@ -311,26 +326,24 @@ export function PMWizard({ * the final Save step. ManifestStepsSection calls useProviderHooks * exactly once regardless of step count (storm fix). */} - {manifestDef && ( - verifyMutation.mutate()} - verificationResult={state.verificationResult} - verifyError={state.verifyError} - hasStoredCredentials={state.hasStoredCredentials} - isEditing={state.isEditing} - /> - )} + verifyMutation.mutate()} + verificationResult={state.verificationResult} + verifyError={state.verifyError} + hasStoredCredentials={state.hasStoredCredentials} + isEditing={state.isEditing} + /> {/* Save slot. */} Date: Sun, 10 May 2026 13:22:36 +0200 Subject: [PATCH 04/15] fix(trello): send card text updates in request body (#1317) Co-authored-by: Cascade Bot --- src/trello/client.ts | 12 ++++++--- tests/unit/trello/client.test.ts | 45 +++++++++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/src/trello/client.ts b/src/trello/client.ts index 94f2c9cd0..1064da0ed 100644 --- a/src/trello/client.ts +++ b/src/trello/client.ts @@ -160,10 +160,14 @@ export const trelloClient = { async updateCard(cardId: string, updates: { name?: string; desc?: string }): Promise { logger.debug('Updating card', { cardId, hasName: !!updates.name, hasDesc: !!updates.desc }); - await getClient().cards.updateCard({ - id: cardId, - name: updates.name, - desc: updates.desc, + const body: { name?: string; desc?: string } = {}; + if (updates.name !== undefined) body.name = updates.name; + if (updates.desc !== undefined) body.desc = updates.desc; + + await trelloFetch(`/cards/${cardId}`, { + method: 'PUT', + headers: { 'Content-Type': 'application/json' }, + body, }); }, diff --git a/tests/unit/trello/client.test.ts b/tests/unit/trello/client.test.ts index 22c5f8f0f..ba4ef81b9 100644 --- a/tests/unit/trello/client.test.ts +++ b/tests/unit/trello/client.test.ts @@ -355,16 +355,53 @@ describe('trelloClient', () => { }); describe('updateCard', () => { - it('calls updateCard with name and desc', async () => { - mockCards.updateCard.mockResolvedValue({}); + it('sends PUT with name and desc in the JSON body', async () => { + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response(JSON.stringify({}), { status: 200 })); await withTrelloCredentials(creds, () => trelloClient.updateCard('card-1', { name: 'New Title', desc: 'New desc' }), ); - expect(mockCards.updateCard).toHaveBeenCalledWith( - expect.objectContaining({ id: 'card-1', name: 'New Title', desc: 'New desc' }), + expect(fetchSpy).toHaveBeenCalledOnce(); + const [url, options] = fetchSpy.mock.calls[0]; + expect(url).toContain('/1/cards/card-1?'); + expect(url).toContain('key=test-key'); + expect(url).toContain('token=test-token'); + expect(options?.method).toBe('PUT'); + expect(options?.headers).toEqual({ 'Content-Type': 'application/json' }); + expect(options?.body).toBe(JSON.stringify({ name: 'New Title', desc: 'New desc' })); + expect(mockCards.updateCard).not.toHaveBeenCalled(); + }); + + it('keeps large descriptions out of the URL', async () => { + const distinctiveText = 'distinctive-long-description-marker'; + const longDescription = `# Long update\n\n${distinctiveText}\n\n${'details '.repeat(2000)}`; + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response(JSON.stringify({}), { status: 200 })); + + await withTrelloCredentials(creds, () => + trelloClient.updateCard('card-1', { name: 'New Title', desc: longDescription }), ); + + const [url, options] = fetchSpy.mock.calls[0]; + expect(String(url)).not.toContain('desc'); + expect(String(url)).not.toContain('New Title'); + expect(String(url)).not.toContain(distinctiveText); + expect(options?.body).toBe(JSON.stringify({ name: 'New Title', desc: longDescription })); + }); + + it('serializes an empty desc so card descriptions can be cleared', async () => { + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response(JSON.stringify({}), { status: 200 })); + + await withTrelloCredentials(creds, () => trelloClient.updateCard('card-1', { desc: '' })); + + const [, options] = fetchSpy.mock.calls[0]; + expect(options?.body).toBe(JSON.stringify({ desc: '' })); }); }); From a767abc3b34890c5122243e7c0b7109446305dac Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 13:44:38 +0200 Subject: [PATCH 05/15] feat(pm-wizard): drive mutation auth from provider metadata (#1318) Co-authored-by: Cascade Bot --- tests/unit/web/pm-wizard-hooks.test.ts | 205 ++++++++++++++++++ .../projects/pm-providers/jira/hooks.ts | 8 +- .../projects/pm-providers/jira/wizard.ts | 10 +- .../projects/pm-providers/linear/hooks.ts | 8 +- .../projects/pm-providers/linear/wizard.ts | 4 +- .../projects/pm-providers/trello/hooks.ts | 14 +- .../projects/pm-providers/trello/wizard.ts | 12 +- .../components/projects/pm-providers/types.ts | 2 + .../components/projects/pm-wizard-hooks.ts | 59 +++-- web/src/components/projects/pm-wizard.tsx | 9 +- 10 files changed, 293 insertions(+), 38 deletions(-) diff --git a/tests/unit/web/pm-wizard-hooks.test.ts b/tests/unit/web/pm-wizard-hooks.test.ts index 56eca3952..35cbef470 100644 --- a/tests/unit/web/pm-wizard-hooks.test.ts +++ b/tests/unit/web/pm-wizard-hooks.test.ts @@ -13,6 +13,8 @@ import { buildIntegrationUpsertInput, buildPersistedCredentialInputs, buildProviderAuthArgFromMetadata, + buildProviderCustomFieldCreationRequest, + buildProviderLabelCreationRequest, runPerLabelCreations, } from '../../../web/src/components/projects/pm-wizard-hooks.js'; import type { WizardState } from '../../../web/src/components/projects/pm-wizard-state.js'; @@ -245,6 +247,209 @@ describe('metadata-driven verification request', () => { }); }); +// ============================================================================ +// Metadata-driven mutation requests +// ============================================================================ + +describe('metadata-driven mutation requests', () => { + function trelloState(overrides: Partial = {}): WizardState { + return { ...createInitialState(), provider: 'trello', trelloBoardId: 'board-1', ...overrides }; + } + function jiraState(overrides: Partial = {}): WizardState { + return { ...createInitialState(), provider: 'jira', jiraProjectKey: 'PROJ', ...overrides }; + } + function linearState(overrides: Partial = {}): WizardState { + return { ...createInitialState(), provider: 'linear', linearTeamId: 'team-1', ...overrides }; + } + + it('builds raw credential payloads for provider label creation', () => { + expect( + buildProviderLabelCreationRequest( + { + providerId: trelloProviderWizard.id, + auth: trelloProviderWizard.auth, + getContainerId: (state) => state.trelloBoardId, + containerError: 'Board must be selected before creating a label', + }, + trelloState({ trelloApiKey: 'key', trelloToken: 'token' }), + 'proj-t', + { name: 'cascade-ready', color: 'sky' }, + ), + ).toEqual({ + providerId: 'trello', + containerId: 'board-1', + name: 'cascade-ready', + color: 'sky', + credentials: { api_key: 'key', token: 'token' }, + }); + expect( + buildProviderLabelCreationRequest( + { + providerId: linearProviderWizard.id, + auth: linearProviderWizard.auth, + getContainerId: (state) => state.linearTeamId, + containerError: 'Team must be selected before creating a label', + }, + linearState({ linearApiKey: 'lin-key' }), + 'proj-l', + { name: 'cascade-ready', color: '#0284C7' }, + ), + ).toEqual({ + providerId: 'linear', + containerId: 'team-1', + name: 'cascade-ready', + color: '#0284C7', + credentials: { api_key: 'lin-key' }, + }); + }); + + it('builds stored credential fallback payloads for provider label creation', () => { + expect( + buildProviderLabelCreationRequest( + { + providerId: trelloProviderWizard.id, + auth: trelloProviderWizard.auth, + getContainerId: (state) => state.trelloBoardId, + containerError: 'Board must be selected before creating a label', + }, + trelloState({ + isEditing: true, + hasStoredCredentials: true, + trelloApiKey: '', + trelloToken: '', + }), + 'proj-t', + { name: 'cascade-ready', color: 'sky' }, + ), + ).toMatchObject({ providerId: 'trello', projectId: 'proj-t' }); + expect( + buildProviderLabelCreationRequest( + { + providerId: linearProviderWizard.id, + auth: linearProviderWizard.auth, + getContainerId: (state) => state.linearTeamId, + containerError: 'Team must be selected before creating a label', + }, + linearState({ isEditing: true, hasStoredCredentials: true, linearApiKey: '' }), + 'proj-l', + { name: 'cascade-ready', color: '#0284C7' }, + ), + ).toMatchObject({ providerId: 'linear', projectId: 'proj-l' }); + }); + + it('builds raw credential payloads for provider custom-field creation', () => { + expect( + buildProviderCustomFieldCreationRequest( + { + providerId: trelloProviderWizard.id, + auth: trelloProviderWizard.auth, + getContainerId: (state) => state.trelloBoardId, + containerError: 'Board must be selected before creating a custom field', + }, + trelloState({ trelloApiKey: 'key', trelloToken: 'token' }), + 'proj-t', + { name: 'Cost' }, + ), + ).toEqual({ + providerId: 'trello', + containerId: 'board-1', + name: 'Cost', + credentials: { api_key: 'key', token: 'token' }, + }); + expect( + buildProviderCustomFieldCreationRequest( + { + providerId: jiraProviderWizard.id, + auth: jiraProviderWizard.auth, + getContainerId: (state) => state.jiraProjectKey || 'global', + }, + jiraState({ + jiraEmail: 'user@example.com', + jiraApiToken: 'jira-token', + jiraBaseUrl: 'https://example.atlassian.net', + }), + 'proj-j', + { name: 'Cost' }, + ), + ).toEqual({ + providerId: 'jira', + containerId: 'PROJ', + name: 'Cost', + credentials: { + email: 'user@example.com', + api_token: 'jira-token', + base_url: 'https://example.atlassian.net', + }, + }); + }); + + it('builds stored credential fallback payloads for provider custom-field creation', () => { + expect( + buildProviderCustomFieldCreationRequest( + { + providerId: trelloProviderWizard.id, + auth: trelloProviderWizard.auth, + getContainerId: (state) => state.trelloBoardId, + containerError: 'Board must be selected before creating a custom field', + }, + trelloState({ + isEditing: true, + hasStoredCredentials: true, + trelloApiKey: '', + trelloToken: '', + }), + 'proj-t', + { name: 'Cost' }, + ), + ).toMatchObject({ providerId: 'trello', projectId: 'proj-t' }); + expect( + buildProviderCustomFieldCreationRequest( + { + providerId: jiraProviderWizard.id, + auth: jiraProviderWizard.auth, + getContainerId: (state) => state.jiraProjectKey || 'global', + }, + jiraState({ + isEditing: true, + hasStoredCredentials: true, + jiraEmail: '', + jiraApiToken: '', + }), + 'proj-j', + { name: 'Cost' }, + ), + ).toMatchObject({ providerId: 'jira', projectId: 'proj-j' }); + }); + + it('throws metadata missing-credential errors for mutation requests', () => { + expect(() => + buildProviderLabelCreationRequest( + { + providerId: linearProviderWizard.id, + auth: linearProviderWizard.auth, + getContainerId: (state) => state.linearTeamId, + containerError: 'Team must be selected before creating a label', + }, + linearState({ linearApiKey: '' }), + 'proj-l', + { name: 'cascade-ready' }, + ), + ).toThrow('Enter your API key before verifying'); + expect(() => + buildProviderCustomFieldCreationRequest( + { + providerId: jiraProviderWizard.id, + auth: jiraProviderWizard.auth, + getContainerId: (state) => state.jiraProjectKey || 'global', + }, + jiraState({ jiraEmail: 'user@example.com', jiraApiToken: 'tok', jiraBaseUrl: '' }), + 'proj-j', + { name: 'Cost' }, + ), + ).toThrow('Enter both credentials before verifying'); + }); +}); + // ============================================================================ // Metadata-driven save // ============================================================================ diff --git a/web/src/components/projects/pm-providers/jira/hooks.ts b/web/src/components/projects/pm-providers/jira/hooks.ts index 6958af3df..f2633f9ec 100644 --- a/web/src/components/projects/pm-providers/jira/hooks.ts +++ b/web/src/components/projects/pm-providers/jira/hooks.ts @@ -4,7 +4,7 @@ import { useEffect } from 'react'; import { trpcClient } from '@/lib/trpc.js'; import { useProviderCustomFieldCreation } from '../../pm-wizard-hooks.js'; import type { WizardAction, WizardState } from '../../pm-wizard-state.js'; -import { jiraAuthMetadata } from './auth.js'; +import type { ProviderAuthMetadata } from '../types.js'; // ============================================================================ // JIRA Discovery @@ -116,14 +116,16 @@ export function useJiraDiscovery( // ============================================================================ export function useJiraCustomFieldCreation( + providerId: string, + auth: ProviderAuthMetadata, state: WizardState, dispatch: Dispatch, projectId: string, ) { const inner = useProviderCustomFieldCreation( { - providerId: 'jira', - auth: jiraAuthMetadata, + providerId, + auth, // JIRA fields are global; containerId is sent as-is for uniform shape getContainerId: (s) => s.jiraProjectKey || 'global', addCustomField: (f) => ({ diff --git a/web/src/components/projects/pm-providers/jira/wizard.ts b/web/src/components/projects/pm-providers/jira/wizard.ts index ba978be39..a0c0c05c9 100644 --- a/web/src/components/projects/pm-providers/jira/wizard.ts +++ b/web/src/components/projects/pm-providers/jira/wizard.ts @@ -313,9 +313,15 @@ export const jiraProviderWizard: ProviderWizardDefinition = { return isCredentialsComplete(state); }, - useProviderHooks: ({ state, dispatch, projectId, advanceToStep }) => { + useProviderHooks: ({ providerId, auth, state, dispatch, projectId, advanceToStep }) => { const discovery = useJiraDiscovery(state, dispatch, advanceToStep, projectId ?? ''); - const customField = useJiraCustomFieldCreation(state, dispatch, projectId ?? ''); + const customField = useJiraCustomFieldCreation( + providerId, + auth, + state, + dispatch, + projectId ?? '', + ); const queryClient = useQueryClient(); const onCreateCustomField = (_slotKey: string, name: string) => { diff --git a/web/src/components/projects/pm-providers/linear/hooks.ts b/web/src/components/projects/pm-providers/linear/hooks.ts index 033d03c0b..899873b37 100644 --- a/web/src/components/projects/pm-providers/linear/hooks.ts +++ b/web/src/components/projects/pm-providers/linear/hooks.ts @@ -10,7 +10,7 @@ import type { WizardAction, WizardState, } from '../../pm-wizard-state.js'; -import { linearAuthMetadata } from './auth.js'; +import type { ProviderAuthMetadata } from '../types.js'; // ============================================================================ // Linear Discovery @@ -156,14 +156,16 @@ export function useLinearDiscovery( // ============================================================================ export function useLinearLabelCreation( + providerId: string, + auth: ProviderAuthMetadata, state: WizardState, dispatch: Dispatch, projectId: string, ) { return useProviderLabelCreation( { - providerId: 'linear', - auth: linearAuthMetadata, + providerId, + auth, getContainerId: (s) => s.linearTeamId, containerError: 'Team must be selected before creating a label', addLabel: (label) => ({ type: 'ADD_LINEAR_TEAM_LABEL', label }), diff --git a/web/src/components/projects/pm-providers/linear/wizard.ts b/web/src/components/projects/pm-providers/linear/wizard.ts index 76eaa8621..cf4997a8c 100644 --- a/web/src/components/projects/pm-providers/linear/wizard.ts +++ b/web/src/components/projects/pm-providers/linear/wizard.ts @@ -283,9 +283,9 @@ export const linearProviderWizard: ProviderWizardDefinition = { return isCredentialsComplete(state); }, - useProviderHooks: ({ state, dispatch, projectId, advanceToStep }) => { + useProviderHooks: ({ providerId, auth, state, dispatch, projectId, advanceToStep }) => { const discovery = useLinearDiscovery(state, dispatch, advanceToStep, projectId ?? ''); - const labels = useLinearLabelCreation(state, dispatch, projectId ?? ''); + const labels = useLinearLabelCreation(providerId, auth, state, dispatch, projectId ?? ''); // Lift the LINEAR_WEBHOOK_SECRET credential lookup from the parent // wizard (`pm-wizard.tsx`) into the provider hooks so the Linear // webhook step adapter can compose the shared `WebhookUrlDisplayStep` diff --git a/web/src/components/projects/pm-providers/trello/hooks.ts b/web/src/components/projects/pm-providers/trello/hooks.ts index b708cc7c3..388e80ae1 100644 --- a/web/src/components/projects/pm-providers/trello/hooks.ts +++ b/web/src/components/projects/pm-providers/trello/hooks.ts @@ -4,7 +4,7 @@ import { useEffect } from 'react'; import { trpcClient } from '@/lib/trpc.js'; import { useProviderCustomFieldCreation, useProviderLabelCreation } from '../../pm-wizard-hooks.js'; import type { WizardAction, WizardState } from '../../pm-wizard-state.js'; -import { trelloAuthMetadata } from './auth.js'; +import type { ProviderAuthMetadata } from '../types.js'; // ============================================================================ // Trello Discovery @@ -113,14 +113,16 @@ export function useTrelloDiscovery( // ============================================================================ export function useTrelloLabelCreation( + providerId: string, + auth: ProviderAuthMetadata, state: WizardState, dispatch: Dispatch, projectId: string, ) { return useProviderLabelCreation( { - providerId: 'trello', - auth: trelloAuthMetadata, + providerId, + auth, getContainerId: (s) => s.trelloBoardId, containerError: 'Board must be selected before creating a label', addLabel: (label) => ({ type: 'ADD_TRELLO_BOARD_LABEL', label }), @@ -137,14 +139,16 @@ export function useTrelloLabelCreation( // ============================================================================ export function useTrelloCustomFieldCreation( + providerId: string, + auth: ProviderAuthMetadata, state: WizardState, dispatch: Dispatch, projectId: string, ) { return useProviderCustomFieldCreation( { - providerId: 'trello', - auth: trelloAuthMetadata, + providerId, + auth, getContainerId: (s) => s.trelloBoardId, containerError: 'Board must be selected before creating a custom field', addCustomField: (f) => ({ type: 'ADD_TRELLO_BOARD_CUSTOM_FIELD', customField: f }), diff --git a/web/src/components/projects/pm-providers/trello/wizard.ts b/web/src/components/projects/pm-providers/trello/wizard.ts index f250e491b..d19aaff6c 100644 --- a/web/src/components/projects/pm-providers/trello/wizard.ts +++ b/web/src/components/projects/pm-providers/trello/wizard.ts @@ -305,10 +305,16 @@ export const trelloProviderWizard: ProviderWizardDefinition = { return isCredentialsComplete(state); }, - useProviderHooks: ({ state, dispatch, projectId, advanceToStep }) => { + useProviderHooks: ({ providerId, auth, state, dispatch, projectId, advanceToStep }) => { const discovery = useTrelloDiscovery(state, dispatch, advanceToStep, projectId ?? ''); - const labels = useTrelloLabelCreation(state, dispatch, projectId ?? ''); - const customField = useTrelloCustomFieldCreation(state, dispatch, projectId ?? ''); + const labels = useTrelloLabelCreation(providerId, auth, state, dispatch, projectId ?? ''); + const customField = useTrelloCustomFieldCreation( + providerId, + auth, + state, + dispatch, + projectId ?? '', + ); const queryClient = useQueryClient(); const [creatingSlot, setCreatingSlot] = useState(null); diff --git a/web/src/components/projects/pm-providers/types.ts b/web/src/components/projects/pm-providers/types.ts index 879009553..7a4b61e70 100644 --- a/web/src/components/projects/pm-providers/types.ts +++ b/web/src/components/projects/pm-providers/types.ts @@ -45,6 +45,8 @@ export interface ProviderWizardStepProps { * provider-specific discovery / label-creation hooks. */ export interface ProviderHooksContext { + readonly providerId: string; + readonly auth: ProviderAuthMetadata; readonly state: WizardState; readonly dispatch: React.Dispatch; readonly projectId: string | undefined; diff --git a/web/src/components/projects/pm-wizard-hooks.ts b/web/src/components/projects/pm-wizard-hooks.ts index b8ccc942c..27061fa2c 100644 --- a/web/src/components/projects/pm-wizard-hooks.ts +++ b/web/src/components/projects/pm-wizard-hooks.ts @@ -117,6 +117,23 @@ interface LabelCreationConfig { setLabelMapping: (slot: string, id: string) => WizardAction; } +export function buildProviderLabelCreationRequest( + config: Pick, + state: WizardState, + projectId: string, + vars: { name: string; color?: string }, +) { + const containerId = config.getContainerId(state); + if (!containerId) throw new Error(config.containerError); + return { + providerId: config.providerId, + containerId, + name: vars.name, + color: vars.color, + ...buildProviderAuthArgFromMetadata(state, projectId, config.auth), + }; +} + function useProviderLabelCreation( config: LabelCreationConfig, state: WizardState, @@ -125,16 +142,8 @@ function useProviderLabelCreation( ) { const createLabelMutation = useMutation({ mutationFn: (vars: { name: string; color?: string; slot: string }) => { - const containerId = config.getContainerId(state); - if (!containerId) throw new Error(config.containerError); - const authArg = buildProviderAuthArgFromMetadata(state, projectId, config.auth); - return trpcClient.pm.discovery.createLabel.mutate({ - providerId: config.providerId, - containerId, - name: vars.name, - color: vars.color, - ...authArg, - }); + const request = buildProviderLabelCreationRequest(config, state, projectId, vars); + return trpcClient.pm.discovery.createLabel.mutate(request); }, onSuccess: (label, vars) => { dispatch(config.addLabel(label)); @@ -202,6 +211,25 @@ interface CustomFieldCreationConfig { onError?: (error: unknown) => void; } +export function buildProviderCustomFieldCreationRequest( + config: Pick< + CustomFieldCreationConfig, + 'providerId' | 'auth' | 'getContainerId' | 'containerError' + >, + state: WizardState, + projectId: string, + vars: { name: string }, +) { + const containerId = config.getContainerId(state); + if (!containerId && config.containerError) throw new Error(config.containerError); + return { + providerId: config.providerId, + containerId: containerId || 'global', + name: vars.name, + ...buildProviderAuthArgFromMetadata(state, projectId, config.auth), + }; +} + function useProviderCustomFieldCreation( config: CustomFieldCreationConfig, state: WizardState, @@ -210,15 +238,8 @@ function useProviderCustomFieldCreation( ) { const createCustomFieldMutation = useMutation({ mutationFn: ({ name }: { name: string }) => { - const containerId = config.getContainerId(state); - if (!containerId && config.containerError) throw new Error(config.containerError); - const authArg = buildProviderAuthArgFromMetadata(state, projectId, config.auth); - return trpcClient.pm.discovery.createCustomField.mutate({ - providerId: config.providerId, - containerId: containerId || 'global', - name, - ...authArg, - }); + const request = buildProviderCustomFieldCreationRequest(config, state, projectId, { name }); + return trpcClient.pm.discovery.createCustomField.mutate(request); }, onSuccess: (customField) => { dispatch( diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index f8105cbf6..559076a77 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -139,7 +139,14 @@ function ManifestStepsSection({ }: ManifestStepsSectionProps) { // Called exactly once — the whole point of this wrapper component. const providerHooks = - manifestDef.useProviderHooks?.({ state, dispatch, projectId, advanceToStep }) ?? {}; + manifestDef.useProviderHooks?.({ + providerId: manifestDef.id, + auth: manifestDef.auth, + state, + dispatch, + projectId, + advanceToStep, + }) ?? {}; return ( <> From 15827e7e1805b8e349663994104cf6e4fc175fcc Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 13:58:30 +0200 Subject: [PATCH 06/15] refactor(web): extract provider wizard state slices (#1319) Co-authored-by: Cascade Bot --- tests/unit/web/pm-wizard-state.test.ts | 100 +++++++++++++ .../projects/pm-providers/jira/state.ts | 65 ++++++++ .../projects/pm-providers/linear/state.ts | 68 +++++++++ .../projects/pm-providers/trello/state.ts | 54 +++++++ .../components/projects/pm-wizard-state.ts | 139 +++++------------- 5 files changed, 325 insertions(+), 101 deletions(-) create mode 100644 web/src/components/projects/pm-providers/jira/state.ts create mode 100644 web/src/components/projects/pm-providers/linear/state.ts create mode 100644 web/src/components/projects/pm-providers/trello/state.ts diff --git a/tests/unit/web/pm-wizard-state.test.ts b/tests/unit/web/pm-wizard-state.test.ts index 09d68a4ae..e56892fa0 100644 --- a/tests/unit/web/pm-wizard-state.test.ts +++ b/tests/unit/web/pm-wizard-state.test.ts @@ -1,4 +1,18 @@ import { describe, expect, it } from 'vitest'; +import { + createInitialJiraState, + INITIAL_JIRA_LABELS as PROVIDER_INITIAL_JIRA_LABELS, + resetJiraProjectState, +} from '../../../web/src/components/projects/pm-providers/jira/state.js'; +import { + createInitialLinearState, + INITIAL_LINEAR_LABELS, + resetLinearTeamState, +} from '../../../web/src/components/projects/pm-providers/linear/state.js'; +import { + createInitialTrelloState, + resetTrelloBoardState, +} from '../../../web/src/components/projects/pm-providers/trello/state.js'; import type { WizardAction, WizardState, @@ -49,6 +63,92 @@ describe('createInitialState', () => { expect(state.isEditing).toBe(false); expect(state.previousProvider).toBeUndefined(); }); + + it('composes provider-owned initial state slices', () => { + const state = createInitialState(); + expect(state).toMatchObject({ + ...createInitialTrelloState(), + ...createInitialJiraState(), + ...createInitialLinearState(), + }); + }); +}); + +// ============================================================================ +// Provider state slices +// ============================================================================ + +describe('provider state slices', () => { + it('owns the Trello initial state defaults', () => { + expect(createInitialTrelloState()).toEqual({ + trelloApiKey: '', + trelloToken: '', + trelloBoardId: '', + trelloBoards: [], + trelloBoardDetails: null, + trelloListMappings: {}, + trelloLabelMappings: {}, + trelloCostFieldId: '', + }); + }); + + it('owns the JIRA initial state defaults', () => { + expect(createInitialJiraState()).toEqual({ + jiraEmail: '', + jiraApiToken: '', + jiraBaseUrl: '', + jiraProjectKey: '', + jiraProjects: [], + jiraProjectDetails: null, + jiraStatusMappings: {}, + jiraIssueTypes: {}, + jiraLabels: PROVIDER_INITIAL_JIRA_LABELS, + jiraCostFieldId: '', + }); + }); + + it('owns the Linear initial state defaults', () => { + expect(createInitialLinearState()).toEqual({ + linearApiKey: '', + linearTeamId: '', + linearTeams: [], + linearProjectId: '', + linearProjects: [], + linearTeamDetails: null, + linearStatusMappings: {}, + linearLabels: INITIAL_LINEAR_LABELS, + }); + }); + + it('captures Trello board-change reset behavior', () => { + expect(resetTrelloBoardState('board-2')).toEqual({ + trelloBoardId: 'board-2', + trelloBoardDetails: null, + trelloListMappings: {}, + trelloLabelMappings: {}, + trelloCostFieldId: '', + }); + }); + + it('captures JIRA project-change reset behavior', () => { + expect(resetJiraProjectState('NEXT')).toEqual({ + jiraProjectKey: 'NEXT', + jiraProjectDetails: null, + jiraStatusMappings: {}, + jiraIssueTypes: {}, + jiraCostFieldId: '', + }); + }); + + it('captures Linear team-change reset behavior, including project scope', () => { + expect(resetLinearTeamState('team-2')).toEqual({ + linearTeamId: 'team-2', + linearTeamDetails: null, + linearStatusMappings: {}, + linearProjectId: '', + linearProjects: [], + }); + }); }); // ============================================================================ diff --git a/web/src/components/projects/pm-providers/jira/state.ts b/web/src/components/projects/pm-providers/jira/state.ts new file mode 100644 index 000000000..d2c339e51 --- /dev/null +++ b/web/src/components/projects/pm-providers/jira/state.ts @@ -0,0 +1,65 @@ +export interface JiraProjectOption { + key: string; + name: string; +} + +export interface JiraProjectDetails { + statuses: Array<{ name: string; id: string }>; + issueTypes: Array<{ name: string; subtask: boolean }>; + fields: Array<{ id: string; name: string; custom: boolean }>; +} + +export const INITIAL_JIRA_LABELS: Record = { + processing: 'cascade-processing', + processed: 'cascade-processed', + error: 'cascade-error', + readyToProcess: 'cascade-ready', + auto: 'cascade-auto', +}; + +export interface JiraWizardStateSlice { + jiraEmail: string; + jiraApiToken: string; + jiraBaseUrl: string; + jiraProjectKey: string; + jiraProjects: JiraProjectOption[]; + jiraProjectDetails: JiraProjectDetails | null; + jiraStatusMappings: Record; + jiraIssueTypes: Record; + jiraLabels: Record; + jiraCostFieldId: string; +} + +export function createInitialJiraState(): JiraWizardStateSlice { + return { + jiraEmail: '', + jiraApiToken: '', + jiraBaseUrl: '', + jiraProjectKey: '', + jiraProjects: [], + jiraProjectDetails: null, + jiraStatusMappings: {}, + jiraIssueTypes: {}, + jiraLabels: { ...INITIAL_JIRA_LABELS }, + jiraCostFieldId: '', + }; +} + +export function resetJiraProjectState( + jiraProjectKey: string, +): Pick< + JiraWizardStateSlice, + | 'jiraProjectKey' + | 'jiraProjectDetails' + | 'jiraStatusMappings' + | 'jiraIssueTypes' + | 'jiraCostFieldId' +> { + return { + jiraProjectKey, + jiraProjectDetails: null, + jiraStatusMappings: {}, + jiraIssueTypes: {}, + jiraCostFieldId: '', + }; +} diff --git a/web/src/components/projects/pm-providers/linear/state.ts b/web/src/components/projects/pm-providers/linear/state.ts new file mode 100644 index 000000000..006ffd36a --- /dev/null +++ b/web/src/components/projects/pm-providers/linear/state.ts @@ -0,0 +1,68 @@ +export interface LinearTeamOption { + id: string; + name: string; + key: string; +} + +export interface LinearProjectOption { + id: string; + name: string; + icon: string | null; + color: string | null; +} + +export interface LinearTeamDetails { + states: Array<{ id: string; name: string; type: string }>; + labels: Array<{ id: string; name: string; color: string }>; +} + +/** + * Linear label mappings store workflow-label **UUIDs**, not names, because + * Linear's GraphQL API rejects names for issueUpdate.labelIds. The wizard + * populates these from the team's existing labels or via the create-label + * button. Initial state is therefore empty — operators pick or create. + */ +export const INITIAL_LINEAR_LABELS: Record = {}; + +export interface LinearWizardStateSlice { + linearApiKey: string; + linearTeamId: string; + linearTeams: LinearTeamOption[]; + linearProjectId: string; + linearProjects: LinearProjectOption[]; + linearTeamDetails: LinearTeamDetails | null; + linearStatusMappings: Record; + linearLabels: Record; +} + +export function createInitialLinearState(): LinearWizardStateSlice { + return { + linearApiKey: '', + linearTeamId: '', + linearTeams: [], + linearProjectId: '', + linearProjects: [], + linearTeamDetails: null, + linearStatusMappings: {}, + linearLabels: { ...INITIAL_LINEAR_LABELS }, + }; +} + +export function resetLinearTeamState( + linearTeamId: string, +): Pick< + LinearWizardStateSlice, + | 'linearTeamId' + | 'linearTeamDetails' + | 'linearStatusMappings' + | 'linearProjectId' + | 'linearProjects' +> { + return { + linearTeamId, + linearTeamDetails: null, + linearStatusMappings: {}, + linearProjectId: '', + linearProjects: [], + }; +} diff --git a/web/src/components/projects/pm-providers/trello/state.ts b/web/src/components/projects/pm-providers/trello/state.ts new file mode 100644 index 000000000..336cdbf1f --- /dev/null +++ b/web/src/components/projects/pm-providers/trello/state.ts @@ -0,0 +1,54 @@ +export interface TrelloBoardOption { + id: string; + name: string; + url: string; +} + +export interface TrelloBoardDetails { + lists: Array<{ id: string; name: string }>; + labels: Array<{ id: string; name: string; color: string }>; + customFields: Array<{ id: string; name: string; type: string }>; +} + +export interface TrelloWizardStateSlice { + trelloApiKey: string; + trelloToken: string; + trelloBoardId: string; + trelloBoards: TrelloBoardOption[]; + trelloBoardDetails: TrelloBoardDetails | null; + trelloListMappings: Record; + trelloLabelMappings: Record; + trelloCostFieldId: string; +} + +export function createInitialTrelloState(): TrelloWizardStateSlice { + return { + trelloApiKey: '', + trelloToken: '', + trelloBoardId: '', + trelloBoards: [], + trelloBoardDetails: null, + trelloListMappings: {}, + trelloLabelMappings: {}, + trelloCostFieldId: '', + }; +} + +export function resetTrelloBoardState( + trelloBoardId: string, +): Pick< + TrelloWizardStateSlice, + | 'trelloBoardId' + | 'trelloBoardDetails' + | 'trelloListMappings' + | 'trelloLabelMappings' + | 'trelloCostFieldId' +> { + return { + trelloBoardId, + trelloBoardDetails: null, + trelloListMappings: {}, + trelloLabelMappings: {}, + trelloCostFieldId: '', + }; +} diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index dc42a3662..ffb5d6b99 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -3,52 +3,43 @@ * Has zero imports from other pm-wizard files to avoid circular dependencies. */ import type { Reducer } from 'react'; +import { + createInitialJiraState, + INITIAL_JIRA_LABELS, + type JiraProjectDetails, + type JiraProjectOption, + resetJiraProjectState, +} from './pm-providers/jira/state.js'; +import { + createInitialLinearState, + INITIAL_LINEAR_LABELS, + type LinearProjectOption, + type LinearTeamDetails, + type LinearTeamOption, + resetLinearTeamState, +} from './pm-providers/linear/state.js'; +import { + createInitialTrelloState, + resetTrelloBoardState, + type TrelloBoardDetails, + type TrelloBoardOption, +} from './pm-providers/trello/state.js'; + +export type { + JiraProjectDetails, + JiraProjectOption, + LinearProjectOption, + LinearTeamDetails, + LinearTeamOption, + TrelloBoardDetails, + TrelloBoardOption, +}; +export { INITIAL_JIRA_LABELS, INITIAL_LINEAR_LABELS }; // ============================================================================ // Types // ============================================================================ -export interface TrelloBoardOption { - id: string; - name: string; - url: string; -} - -export interface TrelloBoardDetails { - lists: Array<{ id: string; name: string }>; - labels: Array<{ id: string; name: string; color: string }>; - customFields: Array<{ id: string; name: string; type: string }>; -} - -export interface JiraProjectOption { - key: string; - name: string; -} - -export interface JiraProjectDetails { - statuses: Array<{ name: string; id: string }>; - issueTypes: Array<{ name: string; subtask: boolean }>; - fields: Array<{ id: string; name: string; custom: boolean }>; -} - -export interface LinearTeamOption { - id: string; - name: string; - key: string; -} - -export interface LinearProjectOption { - id: string; - name: string; - icon: string | null; - color: string | null; -} - -export interface LinearTeamDetails { - states: Array<{ id: string; name: string; type: string }>; - labels: Array<{ id: string; name: string; color: string }>; -} - /** * Provider identifier — an open string so new providers registered via the * frontend barrel (`web/src/components/projects/pm-providers/index.ts`) can @@ -157,53 +148,14 @@ export type WizardAction = // Initial state and constants // ============================================================================ -export const INITIAL_JIRA_LABELS: Record = { - processing: 'cascade-processing', - processed: 'cascade-processed', - error: 'cascade-error', - readyToProcess: 'cascade-ready', - auto: 'cascade-auto', -}; - -/** - * Linear label mappings store workflow-label **UUIDs**, not names, because - * Linear's GraphQL API rejects names for issueUpdate.labelIds. The wizard - * populates these from the team's existing labels or via the create-label - * button. Initial state is therefore empty — operators pick or create. - */ -export const INITIAL_LINEAR_LABELS: Record = {}; - export function createInitialState(): WizardState { return { provider: 'trello', - trelloApiKey: '', - trelloToken: '', - jiraEmail: '', - jiraApiToken: '', - jiraBaseUrl: '', - linearApiKey: '', verificationResult: null, verifyError: null, - trelloBoardId: '', - trelloBoards: [], - jiraProjectKey: '', - jiraProjects: [], - linearTeamId: '', - linearTeams: [], - linearProjectId: '', - linearProjects: [], - trelloBoardDetails: null, - jiraProjectDetails: null, - linearTeamDetails: null, - trelloListMappings: {}, - trelloLabelMappings: {}, - trelloCostFieldId: '', - jiraStatusMappings: {}, - jiraIssueTypes: {}, - jiraLabels: { ...INITIAL_JIRA_LABELS }, - jiraCostFieldId: '', - linearStatusMappings: {}, - linearLabels: { ...INITIAL_LINEAR_LABELS }, + ...createInitialTrelloState(), + ...createInitialJiraState(), + ...createInitialLinearState(), isEditing: false, hasStoredCredentials: false, }; @@ -268,36 +220,21 @@ export const wizardReducer: Reducer = (state, action) case 'SET_TRELLO_BOARD_ID': return { ...state, - trelloBoardId: action.id, - trelloBoardDetails: null, - trelloListMappings: {}, - trelloLabelMappings: {}, - trelloCostFieldId: '', + ...resetTrelloBoardState(action.id), }; case 'SET_JIRA_PROJECTS': return { ...state, jiraProjects: action.projects }; case 'SET_JIRA_PROJECT_KEY': return { ...state, - jiraProjectKey: action.key, - jiraProjectDetails: null, - jiraStatusMappings: {}, - jiraIssueTypes: {}, - jiraCostFieldId: '', + ...resetJiraProjectState(action.key), }; case 'SET_LINEAR_TEAMS': return { ...state, linearTeams: action.teams }; case 'SET_LINEAR_TEAM_ID': return { ...state, - linearTeamId: action.id, - linearTeamDetails: null, - linearStatusMappings: {}, - // A new team invalidates the project list and any chosen project — - // Linear projects are team-scoped, so the previous selection is - // not guaranteed to belong to the new team. - linearProjectId: '', - linearProjects: [], + ...resetLinearTeamState(action.id), }; case 'SET_LINEAR_PROJECTS': return { ...state, linearProjects: action.projects }; From 6f78a562dc00ab1df13fdcb5c63180b3f424f3b5 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 14:23:22 +0200 Subject: [PATCH 07/15] feat(pm-wizard): move edit hydration into provider definitions (#1320) Co-authored-by: Cascade Bot --- CLAUDE.md | 2 +- src/integrations/README.md | 9 ++- tests/unit/web/pm-provider-registry.test.ts | 2 + .../web/pm-wizard-generic-renderer.test.ts | 29 ++++++- tests/unit/web/pm-wizard-state.test.ts | 40 +++++----- .../components/projects/pm-providers/index.ts | 7 +- .../projects/pm-providers/jira/wizard.ts | 19 +++++ .../projects/pm-providers/linear/wizard.ts | 14 ++++ .../projects/pm-providers/trello/wizard.ts | 22 ++++- .../components/projects/pm-providers/types.ts | 9 +++ .../components/projects/pm-wizard-state.ts | 80 ++----------------- web/src/components/projects/pm-wizard.tsx | 19 ++++- 12 files changed, 142 insertions(+), 110 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 661bb6bb3..51d4ff117 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -25,7 +25,7 @@ Flow: `PM/SCM/alerting webhook → Router → Redis → Worker → TriggerRegist **Capacity-gate invariant.** Every PM router adapter (`src/router/adapters/{linear,trello,jira}.ts`) must wrap `triggerRegistry.dispatch(ctx)` in PM-provider `AsyncLocalStorage` scope via the shared `withPMScopeForDispatch(fullProject, dispatch)` helper at `src/router/adapters/_shared.ts` — in addition to the per-PM-type credential scope (`withLinearCredentials` / `withTrelloCredentials` / `withJiraCredentials`). Without the PM-provider wrapping, the pipeline-capacity gate at `src/triggers/shared/pipeline-capacity-gate.ts` cannot resolve `getPMProvider()`, **fails closed** under the spec-017 fail-closed policy (blocks the run + ERROR + Sentry capture under tag `pipeline_capacity_gate_no_pm_provider`), and `maxInFlightItems` is silently disabled for the PM-source path. Mirror the GitHub adapter's existing correct shape at `src/router/adapters/github.ts:dispatchWithCredentials`. The static guard at `tests/unit/integrations/pm-router-adapter-pm-scope.test.ts` enforces this at CI time — adding a new PM router adapter without the wrapping fails CI with a precise file path. -Integration abstraction lives in `src/integrations/`. For **adding a new PM provider**, see @src/integrations/README.md — PM providers (Trello, JIRA, Linear) use the `PMProviderManifest` registry with a **behavioral conformance harness** (spec 009 — config round-trip, discovery shape, full lifecycle scenario, auth-header provenance, single-entrypoint invariant). Each provider owns its Zod config schema (`src/integrations/pm//config-schema.ts`) as the single source of truth — the central `src/config/schema.ts` imports it. PM adapter method signatures use branded `StateId` / `LabelId` / `ContainerId` from `src/pm/ids.ts` to make state-name-vs-ID confusion a compile error at direct-adapter call sites. All runtime surfaces (router, worker, CLI, dashboard) register integrations through a single entrypoint at `src/integrations/entrypoint.ts`. **Spec 010 follow-ups** added generic `pm.discovery.createLabel` / `createCustomField` mutation endpoints + `currentUser` discovery capability + real shared React components for every `StandardStepKind` under `web/src/components/projects/pm-providers/steps/`. **Spec 011** migrated all three production providers (Trello, JIRA, Linear) onto those shared components, added a 7th `StandardStepKind: custom-field-mapping`, widened `container-pick` / `project-scope` / `webhook-url-display` with optional props, and deleted the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` files. **Spec 012** migrated each provider's webhook UX (programmatic create for Trello/JIRA, signing-secret + instructions for Linear) into per-provider manifest webhook adapters (Fragment compositions around the shared `WebhookUrlDisplayStep`); deleted the legacy `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo`. Every PM wizard step now renders via the manifest path without exception. A new PM provider writes one import in the backend barrel (`src/integrations/pm/index.ts`) and one import in the frontend barrel (`web/src/components/projects/pm-providers/index.ts`); `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, and `pm-wizard-hooks.ts` receive zero edits. The verification-button readiness path (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are metadata-driven and require no changes for a new provider. **The shared dashboard state (`pm-wizard-state.ts`) does still require edits**: new providers must add their credential fields to `WizardState` (e.g. `asanaApiKey: string`), the corresponding action types to `WizardAction`, and the config-shape handling in `buildEditState` — see step 3 of @src/integrations/README.md. Provider-specific hooks, auth metadata, verification display formatting, and UI live inside the provider folder (`kind: 'custom'` steps or Fragment compositions around shared steps). SCM (GitHub) and alerting (Sentry) still use the legacy `IntegrationModule` pattern via self-registration in `src/github/register.ts` + `src/sentry/register.ts`. Don't improvise; the README covers both patterns. +Integration abstraction lives in `src/integrations/`. For **adding a new PM provider**, see @src/integrations/README.md — PM providers (Trello, JIRA, Linear) use the `PMProviderManifest` registry with a **behavioral conformance harness** (spec 009 — config round-trip, discovery shape, full lifecycle scenario, auth-header provenance, single-entrypoint invariant). Each provider owns its Zod config schema (`src/integrations/pm//config-schema.ts`) as the single source of truth — the central `src/config/schema.ts` imports it. PM adapter method signatures use branded `StateId` / `LabelId` / `ContainerId` from `src/pm/ids.ts` to make state-name-vs-ID confusion a compile error at direct-adapter call sites. All runtime surfaces (router, worker, CLI, dashboard) register integrations through a single entrypoint at `src/integrations/entrypoint.ts`. **Spec 010 follow-ups** added generic `pm.discovery.createLabel` / `createCustomField` mutation endpoints + `currentUser` discovery capability + real shared React components for every `StandardStepKind` under `web/src/components/projects/pm-providers/steps/`. **Spec 011** migrated all three production providers (Trello, JIRA, Linear) onto those shared components, added a 7th `StandardStepKind: custom-field-mapping`, widened `container-pick` / `project-scope` / `webhook-url-display` with optional props, and deleted the three legacy `pm-wizard-{trello,jira,linear}-steps.tsx` files. **Spec 012** migrated each provider's webhook UX (programmatic create for Trello/JIRA, signing-secret + instructions for Linear) into per-provider manifest webhook adapters (Fragment compositions around the shared `WebhookUrlDisplayStep`); deleted the legacy `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo`. Every PM wizard step now renders via the manifest path without exception. A new PM provider writes one import in the backend barrel (`src/integrations/pm/index.ts`) and one import in the frontend barrel (`web/src/components/projects/pm-providers/index.ts`); `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, and `pm-wizard-hooks.ts` receive zero edits. The verification-button readiness path (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are metadata-driven and require no changes for a new provider. **The shared dashboard state (`pm-wizard-state.ts`) does still require edits**: new providers must add their credential fields to `WizardState` (e.g. `asanaApiKey: string`) and the corresponding action types to `WizardAction`; config-shape hydration belongs on the provider's `ProviderWizardDefinition.buildEditState` — see step 4 of @src/integrations/README.md. Provider-specific hooks, auth metadata, verification display formatting, and UI live inside the provider folder (`kind: 'custom'` steps or Fragment compositions around shared steps). SCM (GitHub) and alerting (Sentry) still use the legacy `IntegrationModule` pattern via self-registration in `src/github/register.ts` + `src/sentry/register.ts`. Don't improvise; the README covers both patterns. ## PR checkout (worker) — gotcha diff --git a/src/integrations/README.md b/src/integrations/README.md index fde166858..45e371a32 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -204,13 +204,13 @@ All three real providers are now on the hardened contracts. Plan 009/4 also ship | Webhook-UX migration complete | Every PM wizard step, without exception, renders via the manifest path. Trello, JIRA, and Linear each own their webhook step via a per-provider adapter (`pm-providers//webhook-step.tsx`) — Fragment composition around the shared `WebhookUrlDisplayStep`. Trello + JIRA compose with programmatic "Create Webhook" button + active-webhooks list + delete + curl fallback (via existing `webhooks.create/list/delete({trelloOnly|jiraOnly:true})` tRPC endpoints). Linear composes with info banner + `ProjectSecretField` (`LINEAR_WEBHOOK_SECRET`) + 5-step manual setup instructions. | | Legacy deletions | `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo` all deleted. `pm-wizard-common-steps.tsx` now only exports `SaveStep`. Legacy test file `pm-wizard-webhooks-step.test.ts` deleted — assertions moved into per-provider adapter tests. | | Parent-wizard filter | The `-webhook` id-skip filter (stopgap from plan 011/4) is gone. `renderedManifestSteps = manifestDef.steps.map(...)` — no filter. | -| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. New providers add one import to the frontend barrel (`web/src/components/projects/pm-providers/index.ts`) — the symmetric counterpart of the backend barrel — and `pm-wizard.tsx` picks it up automatically. The provider picker, verification-button readiness (`areCredentialsReadyFromMetadata`), and mutation auth path (`buildProviderAuthArgFromMetadata`) are all metadata-driven; no shared edits required beyond the barrel import. **Shared dashboard state** (`pm-wizard-state.ts`) must still be updated with the new provider's credential fields (`WizardState`), reducer action types, and `buildEditState` handling — see step 3 of "Adding a new PM provider" below. | +| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. New providers add one import to the frontend barrel (`web/src/components/projects/pm-providers/index.ts`) — the symmetric counterpart of the backend barrel — and `pm-wizard.tsx` picks it up automatically. The provider picker, edit hydration dispatch (`ProviderWizardDefinition.buildEditState`), verification-button readiness (`areCredentialsReadyFromMetadata`), and mutation auth path (`buildProviderAuthArgFromMetadata`) are all metadata-driven; no shared edits required beyond the barrel import. **Shared dashboard state** (`pm-wizard-state.ts`) must still be updated with the new provider's credential fields (`WizardState`) and reducer action types — see step 4 of "Adding a new PM provider" below. | --- ## Adding a new PM provider (step by step) -Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. The orchestration and schema work lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The one shared dashboard file that still requires a new-provider edit is `pm-wizard-state.ts` (step 4 below) — new providers add credential fields to `WizardState`, action types to `WizardAction`, and a `buildEditState` branch. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces the shared-file invariant for orchestration and schema files; `pm-wizard-state.ts` is the deliberate exception. +Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. The orchestration and schema work lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The one shared dashboard file that still requires a new-provider edit is `pm-wizard-state.ts` (step 4 below) — new providers add credential fields to `WizardState` and action types to `WizardAction`. Edit-mode config hydration belongs on the provider's `ProviderWizardDefinition.buildEditState`. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces the shared-file invariant for orchestration and schema files; `pm-wizard-state.ts` is the deliberate exception. 1. **Backend folder** at `src/integrations/pm//`: - `client.ts` (or reuse a sibling under `src//`) — your REST / GraphQL client. Must use `withXxxCredentials()` + AsyncLocalStorage credential scoping; never hand-assemble Bearer headers (see `_shared/auth-headers.ts`). @@ -227,7 +227,8 @@ Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / 4. **Update shared dashboard state** in `web/src/components/projects/pm-wizard-state.ts`. This is the one shared dashboard file a new provider must edit: - Add the provider's raw credential fields to `WizardState` (e.g. `asanaApiKey: string`). - Add the corresponding `SET__` action types to `WizardAction` (one per credential field); each reducer case should set `verificationResult: null, verifyError: null` alongside the updated field. - - Add an `else if (provider === '')` branch to `buildEditState` that sets `hasStoredCredentials` based on `configuredKeys.has('')` and restores any saved config values (container IDs, status/label mappings). + + Edit-mode hydration is provider-owned: implement `buildEditState(initialConfig, configuredKeys)` on the provider's `ProviderWizardDefinition`. It should set `hasStoredCredentials` from the relevant persisted credential keys and restore saved config values (container IDs, status/label mappings) without returning raw credential values. The credential-readiness check (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are fully metadata-driven — they read `manifestDef.auth.rawCredentials` and require **no changes** here. @@ -239,7 +240,7 @@ Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / 8. **Provider-specific unit tests** in `tests/unit/pm//` — adapter tests (vi.mock the client), config-schema round-trip, discovery shape, wizardSpec, adapter branded IDs. -The shared orchestration files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, `pm-wizard-common-steps.tsx`) require zero edits beyond the barrel import in step 5. The `new-provider-surface` snapshot test proves your PR does not modify shared router / worker / CLI / dashboard orchestration or central schema files. The one deliberate shared-dashboard exception is `pm-wizard-state.ts` (step 4 above). +The shared orchestration files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, `pm-wizard-common-steps.tsx`) require zero edits beyond the barrel import in step 5. The `new-provider-surface` snapshot test proves your PR does not modify shared router / worker / CLI / dashboard orchestration or central schema files. The one deliberate shared-dashboard exception is `pm-wizard-state.ts` for provider-specific state fields and reducer actions (step 4 above). --- diff --git a/tests/unit/web/pm-provider-registry.test.ts b/tests/unit/web/pm-provider-registry.test.ts index a6e9267a8..78ca484a2 100644 --- a/tests/unit/web/pm-provider-registry.test.ts +++ b/tests/unit/web/pm-provider-registry.test.ts @@ -20,7 +20,9 @@ function makeStubWizard(id: string): ProviderWizardDefinition { credentialPersistence: [ { envVarKey: 'STUB_API_KEY', stateField: 'linearApiKey', label: 'Stub API Key' }, ], + formatVerificationDisplay: (me) => me.displayName ?? me.name, buildIntegrationConfig: () => ({}), + buildEditState: () => ({ provider: id }), isSetupComplete: () => true, }; } diff --git a/tests/unit/web/pm-wizard-generic-renderer.test.ts b/tests/unit/web/pm-wizard-generic-renderer.test.ts index 7409e0879..ddd4473af 100644 --- a/tests/unit/web/pm-wizard-generic-renderer.test.ts +++ b/tests/unit/web/pm-wizard-generic-renderer.test.ts @@ -8,12 +8,14 @@ import { import type { ProviderWizardDefinition } from '../../../web/src/components/projects/pm-providers/types.js'; import type { buildProviderSwitchConfirmationMessage as BuildConfirmationMessage, + buildEditStateFromProviderWizard as BuildEditStateFromProviderWizard, ProviderPicker as ProviderPickerComponent, } from '../../../web/src/components/projects/pm-wizard.js'; import { createInitialState } from '../../../web/src/components/projects/pm-wizard-state.js'; let ProviderPicker: typeof ProviderPickerComponent; let buildProviderSwitchConfirmationMessage: typeof BuildConfirmationMessage; +let buildEditStateFromProviderWizard: typeof BuildEditStateFromProviderWizard; function StubStep({ state }: { state: { provider: string } }) { return createElement( @@ -40,6 +42,10 @@ function makeStubWizard(id: string, label: string): ProviderWizardDefinition { { envVarKey: 'STUB_API_KEY', stateField: 'linearApiKey', label: 'Stub API Key' }, ], buildIntegrationConfig: () => ({}), + buildEditState: (_initialConfig, configuredKeys) => ({ + provider: id, + hasStoredCredentials: configuredKeys.has('STUB_API_KEY'), + }), isSetupComplete: () => true, }; } @@ -56,9 +62,8 @@ function renderProviderPicker() { describe('PMWizard provider picker', () => { beforeAll(async () => { - ({ ProviderPicker, buildProviderSwitchConfirmationMessage } = await import( - '../../../web/src/components/projects/pm-wizard.js' - )); + ({ ProviderPicker, buildProviderSwitchConfirmationMessage, buildEditStateFromProviderWizard } = + await import('../../../web/src/components/projects/pm-wizard.js')); }); beforeEach(() => { @@ -88,4 +93,22 @@ describe('PMWizard provider picker', () => { "Switch PM provider from Trello to Acme PM?\n\nYou'll need to re-enter credentials and re-map fields for Acme PM. The old provider's credentials will be deleted when you save.", ); }); + + it('builds edit state through the registered provider wizard definition', () => { + registerProviderWizard(makeStubWizard('acme', 'Acme PM')); + + const result = buildEditStateFromProviderWizard( + 'acme', + { ignored: true }, + new Set(['STUB_API_KEY']), + ); + + expect(result).toEqual({ provider: 'acme', hasStoredCredentials: true }); + }); + + it('throws explicitly when edit state is requested for an unknown provider', () => { + expect(() => buildEditStateFromProviderWizard('unknown', {}, new Set())).toThrowError( + 'No PM provider wizard registered for unknown', + ); + }); }); diff --git a/tests/unit/web/pm-wizard-state.test.ts b/tests/unit/web/pm-wizard-state.test.ts index e56892fa0..64ee0ee97 100644 --- a/tests/unit/web/pm-wizard-state.test.ts +++ b/tests/unit/web/pm-wizard-state.test.ts @@ -4,22 +4,24 @@ import { INITIAL_JIRA_LABELS as PROVIDER_INITIAL_JIRA_LABELS, resetJiraProjectState, } from '../../../web/src/components/projects/pm-providers/jira/state.js'; +import { jiraProviderWizard } from '../../../web/src/components/projects/pm-providers/jira/wizard.js'; import { createInitialLinearState, INITIAL_LINEAR_LABELS, resetLinearTeamState, } from '../../../web/src/components/projects/pm-providers/linear/state.js'; +import { linearProviderWizard } from '../../../web/src/components/projects/pm-providers/linear/wizard.js'; import { createInitialTrelloState, resetTrelloBoardState, } from '../../../web/src/components/projects/pm-providers/trello/state.js'; +import { trelloProviderWizard } from '../../../web/src/components/projects/pm-providers/trello/wizard.js'; import type { WizardAction, WizardState, } from '../../../web/src/components/projects/pm-wizard-state.js'; import { areCredentialsReady, - buildEditState, buildLinearIntegrationConfig, createInitialState, deriveActiveWebhooks, @@ -819,10 +821,10 @@ describe('shouldUseStoredCredentials', () => { }); // ============================================================================ -// buildEditState +// ProviderWizardDefinition.buildEditState // ============================================================================ -describe('buildEditState', () => { +describe('provider-owned edit hydration', () => { it('builds trello edit state from config', () => { const config = { boardId: 'board-abc', @@ -830,7 +832,7 @@ describe('buildEditState', () => { labels: { processing: 'label-x' }, customFields: { cost: 'cf-cost-1' }, }; - const result = buildEditState('trello', config, new Set()); + const result = trelloProviderWizard.buildEditState(config, new Set()); expect(result.provider).toBe('trello'); // Raw credential values are NOT pre-populated for security expect(result.trelloApiKey).toBeUndefined(); @@ -843,17 +845,20 @@ describe('buildEditState', () => { it('sets hasStoredCredentials true for trello when both keys present', () => { const config = { boardId: 'board-abc' }; - const result = buildEditState('trello', config, new Set(['TRELLO_API_KEY', 'TRELLO_TOKEN'])); + const result = trelloProviderWizard.buildEditState( + config, + new Set(['TRELLO_API_KEY', 'TRELLO_TOKEN']), + ); expect(result.hasStoredCredentials).toBe(true); }); it('sets hasStoredCredentials false for trello when only one key present', () => { - const result = buildEditState('trello', {}, new Set(['TRELLO_API_KEY'])); + const result = trelloProviderWizard.buildEditState({}, new Set(['TRELLO_API_KEY'])); expect(result.hasStoredCredentials).toBe(false); }); it('sets hasStoredCredentials false for trello when no keys present', () => { - const result = buildEditState('trello', {}, new Set()); + const result = trelloProviderWizard.buildEditState({}, new Set()); expect(result.hasStoredCredentials).toBe(false); }); @@ -866,7 +871,7 @@ describe('buildEditState', () => { labels: { processing: 'cascade-processing' }, customFields: { cost: 'customfield_10042' }, }; - const result = buildEditState('jira', config, new Set()); + const result = jiraProviderWizard.buildEditState(config, new Set()); expect(result.provider).toBe('jira'); // Raw credential values are NOT pre-populated for security expect(result.jiraEmail).toBeUndefined(); @@ -880,8 +885,7 @@ describe('buildEditState', () => { }); it('sets hasStoredCredentials true for jira when both keys present', () => { - const result = buildEditState( - 'jira', + const result = jiraProviderWizard.buildEditState( { baseUrl: 'https://example.atlassian.net', projectKey: 'PROJ' }, new Set(['JIRA_EMAIL', 'JIRA_API_TOKEN']), ); @@ -889,23 +893,17 @@ describe('buildEditState', () => { }); it('sets hasStoredCredentials false for jira when only one key present', () => { - const result = buildEditState('jira', {}, new Set(['JIRA_EMAIL'])); + const result = jiraProviderWizard.buildEditState({}, new Set(['JIRA_EMAIL'])); expect(result.hasStoredCredentials).toBe(false); }); it('handles missing optional config fields gracefully', () => { const config = { boardId: 'board-1' }; - const result = buildEditState('trello', config, new Set()); + const result = trelloProviderWizard.buildEditState(config, new Set()); expect(result.trelloBoardId).toBe('board-1'); expect(result.trelloListMappings).toBeUndefined(); expect(result.trelloCostFieldId).toBe(''); }); - - it('returns only provider for unknown provider', () => { - const result = buildEditState('unknown', {}, new Set()); - expect(result.provider).toBe('unknown'); - expect(Object.keys(result).length).toBe(1); - }); }); // ============================================================================ @@ -983,8 +981,7 @@ describe('Linear project scope — wizardReducer', () => { describe('Linear project scope — buildEditState hydration', () => { it('hydrates linearProjectId from initialConfig.projectId when present', () => { - const result = buildEditState( - 'linear', + const result = linearProviderWizard.buildEditState( { teamId: 'T1', projectId: 'P1', statuses: {} }, new Set(['LINEAR_API_KEY']), ); @@ -992,8 +989,7 @@ describe('Linear project scope — buildEditState hydration', () => { }); it('leaves linearProjectId unset when initialConfig has no projectId', () => { - const result = buildEditState( - 'linear', + const result = linearProviderWizard.buildEditState( { teamId: 'T1', statuses: {} }, new Set(['LINEAR_API_KEY']), ); diff --git a/web/src/components/projects/pm-providers/index.ts b/web/src/components/projects/pm-providers/index.ts index b4116bc5e..78278ef0b 100644 --- a/web/src/components/projects/pm-providers/index.ts +++ b/web/src/components/projects/pm-providers/index.ts @@ -13,9 +13,10 @@ * `pm-wizard-common-steps.tsx`) need zero edits after this import. * The one shared dashboard file that still requires manual edits is * `pm-wizard-state.ts` — new providers must add their credential fields - * to `WizardState`, the corresponding action types to `WizardAction`, and - * `buildEditState` handling for their config shape. See step 4 of - * "Adding a new PM provider" in src/integrations/README.md. + * to `WizardState` and the corresponding action types to `WizardAction`. + * Provider-specific edit hydration belongs on the provider's + * `ProviderWizardDefinition.buildEditState`. See step 4 of "Adding a new PM + * provider" in src/integrations/README.md. */ import './trello/index.js'; diff --git a/web/src/components/projects/pm-providers/jira/wizard.ts b/web/src/components/projects/pm-providers/jira/wizard.ts index a0c0c05c9..b529e9b1d 100644 --- a/web/src/components/projects/pm-providers/jira/wizard.ts +++ b/web/src/components/projects/pm-providers/jira/wizard.ts @@ -307,6 +307,25 @@ export const jiraProviderWizard: ProviderWizardDefinition = { ...(state.jiraCostFieldId ? { customFields: { cost: state.jiraCostFieldId } } : {}), }), + buildEditState: (initialConfig, configuredKeys) => { + const statuses = initialConfig.statuses as Record | undefined; + const issueTypes = initialConfig.issueTypes as Record | undefined; + const labels = initialConfig.labels as Record | undefined; + + return { + provider: 'jira', + jiraBaseUrl: (initialConfig.baseUrl as string) ?? '', + jiraProjectKey: (initialConfig.projectKey as string) ?? '', + ...(statuses ? { jiraStatusMappings: statuses } : {}), + ...(issueTypes ? { jiraIssueTypes: issueTypes } : {}), + ...(labels ? { jiraLabels: labels } : {}), + jiraCostFieldId: + (initialConfig.customFields as Record | undefined)?.cost ?? '', + hasStoredCredentials: + configuredKeys.has('JIRA_EMAIL') && configuredKeys.has('JIRA_API_TOKEN'), + }; + }, + isSetupComplete: (state) => { if (!state.jiraProjectKey) return false; if (Object.keys(state.jiraStatusMappings).length === 0) return false; diff --git a/web/src/components/projects/pm-providers/linear/wizard.ts b/web/src/components/projects/pm-providers/linear/wizard.ts index cf4997a8c..469328f3a 100644 --- a/web/src/components/projects/pm-providers/linear/wizard.ts +++ b/web/src/components/projects/pm-providers/linear/wizard.ts @@ -277,6 +277,20 @@ export const linearProviderWizard: ProviderWizardDefinition = { buildIntegrationConfig: buildLinearIntegrationConfig, + buildEditState: (initialConfig, configuredKeys) => { + const statuses = initialConfig.statuses as Record | undefined; + const labels = initialConfig.labels as Record | undefined; + + return { + provider: 'linear', + linearTeamId: (initialConfig.teamId as string) ?? '', + linearProjectId: (initialConfig.projectId as string) ?? '', + ...(statuses ? { linearStatusMappings: statuses } : {}), + ...(labels ? { linearLabels: labels } : {}), + hasStoredCredentials: configuredKeys.has('LINEAR_API_KEY'), + }; + }, + isSetupComplete: (state) => { if (!state.linearTeamId) return false; if (Object.keys(state.linearStatusMappings).length === 0) return false; diff --git a/web/src/components/projects/pm-providers/trello/wizard.ts b/web/src/components/projects/pm-providers/trello/wizard.ts index d19aaff6c..28a52d740 100644 --- a/web/src/components/projects/pm-providers/trello/wizard.ts +++ b/web/src/components/projects/pm-providers/trello/wizard.ts @@ -21,7 +21,7 @@ import type { ReactElement } from 'react'; import { useState } from 'react'; import { API_URL } from '@/lib/api.js'; import { trpc, trpcClient } from '@/lib/trpc.js'; -import { deriveActiveWebhooks } from '../../pm-wizard-state.js'; +import { deriveActiveWebhooks, type WizardState } from '../../pm-wizard-state.js'; import { ContainerPickStep } from '../steps/container-pick.js'; import { CustomFieldMappingStep } from '../steps/custom-field-mapping.js'; import { LabelMappingStep } from '../steps/label-mapping.js'; @@ -299,6 +299,26 @@ export const trelloProviderWizard: ProviderWizardDefinition = { ...(state.trelloCostFieldId ? { customFields: { cost: state.trelloCostFieldId } } : {}), }), + buildEditState: (initialConfig, configuredKeys) => { + const editState = { + provider: 'trello', + trelloBoardId: (initialConfig.boardId as string) ?? '', + trelloCostFieldId: + (initialConfig.customFields as Record | undefined)?.cost ?? '', + hasStoredCredentials: + configuredKeys.has('TRELLO_API_KEY') && configuredKeys.has('TRELLO_TOKEN'), + } satisfies Partial; + + const lists = initialConfig.lists as Record | undefined; + const labels = initialConfig.labels as Record | undefined; + + return { + ...editState, + ...(lists ? { trelloListMappings: lists } : {}), + ...(labels ? { trelloLabelMappings: labels } : {}), + }; + }, + isSetupComplete: (state) => { if (!state.trelloBoardId) return false; if (Object.keys(state.trelloListMappings).length === 0) return false; diff --git a/web/src/components/projects/pm-providers/types.ts b/web/src/components/projects/pm-providers/types.ts index 7a4b61e70..9a5483da1 100644 --- a/web/src/components/projects/pm-providers/types.ts +++ b/web/src/components/projects/pm-providers/types.ts @@ -110,6 +110,15 @@ export interface ProviderWizardDefinition { * save API. Mirrors the existing `buildXxxIntegrationConfig` functions. */ readonly buildIntegrationConfig: (state: WizardState) => Record; + /** + * Hydrates provider-owned edit-mode wizard state from a saved integration + * config plus the project credential keys currently configured on the server. + * Raw credential values must not be returned. + */ + readonly buildEditState: ( + initialConfig: Record, + configuredKeys: ReadonlySet, + ) => Partial; /** True when all required steps report complete. */ readonly isSetupComplete: (state: WizardState) => boolean; /** diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index ffb5d6b99..a5a122a37 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -46,9 +46,10 @@ export { INITIAL_JIRA_LABELS, INITIAL_LINEAR_LABELS }; * be dispatched without adding to a closed union here. * * Note: adding a new PM provider still requires updating `WizardState` with - * the provider's credential fields (e.g. `asanaApiKey: string`), the reducer - * with the corresponding action types, and `buildEditState` with the new - * provider's config-shape handling. The credential-readiness path + * the provider's credential fields (e.g. `asanaApiKey: string`) and the + * reducer with the corresponding action types. Provider config-shape hydration + * belongs on that provider's `ProviderWizardDefinition.buildEditState`. + * The credential-readiness path * (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the * mutation auth path (`buildProviderAuthArgFromMetadata`) are metadata-driven * and do NOT require changes. @@ -332,75 +333,6 @@ export const wizardReducer: Reducer = (state, action) } }; -// ============================================================================ -// Edit-mode state builder -// ============================================================================ - -/** - * Build a partial WizardState from an existing integration's config. - * Called when editing an existing PM integration. - * Note: Raw credential values are NOT pre-populated for security. When stored credentials - * exist in project_credentials, `hasStoredCredentials` is set true so the wizard can - * operate without re-entry. - */ -// biome-ignore lint/complexity/noExcessiveCognitiveComplexity: restoring state from two provider config shapes -export function buildEditState( - provider: string, - initialConfig: Record, - configuredKeys: Set, -): Partial { - const editState: Partial = { - provider: provider as Provider, - }; - - if (provider === 'trello') { - editState.trelloBoardId = (initialConfig.boardId as string) ?? ''; - - const lists = initialConfig.lists as Record | undefined; - if (lists) editState.trelloListMappings = lists; - - const labels = initialConfig.labels as Record | undefined; - if (labels) editState.trelloLabelMappings = labels; - - const cf = initialConfig.customFields as Record | undefined; - editState.trelloCostFieldId = cf?.cost ?? ''; - - editState.hasStoredCredentials = - configuredKeys.has('TRELLO_API_KEY') && configuredKeys.has('TRELLO_TOKEN'); - } else if (provider === 'jira') { - editState.jiraBaseUrl = (initialConfig.baseUrl as string) ?? ''; - editState.jiraProjectKey = (initialConfig.projectKey as string) ?? ''; - - const statuses = initialConfig.statuses as Record | undefined; - if (statuses) editState.jiraStatusMappings = statuses; - - const issueTypes = initialConfig.issueTypes as Record | undefined; - if (issueTypes) editState.jiraIssueTypes = issueTypes; - - const labels = initialConfig.labels as Record | undefined; - if (labels) editState.jiraLabels = labels; - - const cf = initialConfig.customFields as Record | undefined; - editState.jiraCostFieldId = cf?.cost ?? ''; - - editState.hasStoredCredentials = - configuredKeys.has('JIRA_EMAIL') && configuredKeys.has('JIRA_API_TOKEN'); - } else if (provider === 'linear') { - editState.linearTeamId = (initialConfig.teamId as string) ?? ''; - editState.linearProjectId = (initialConfig.projectId as string) ?? ''; - - const statuses = initialConfig.statuses as Record | undefined; - if (statuses) editState.linearStatusMappings = statuses; - - const labels = initialConfig.labels as Record | undefined; - if (labels) editState.linearLabels = labels; - - editState.hasStoredCredentials = configuredKeys.has('LINEAR_API_KEY'); - } - - return editState; -} - // ============================================================================ // Step-completion helpers (pure functions) // ============================================================================ @@ -443,8 +375,8 @@ export function areCredentialsReady(state: WizardState): boolean { * Returns `true` when a wizard mutation (verify, createLabel, createCustomField) * should pass `projectId` to the backend — meaning: edit mode is active, the * provider has stored credentials in `project_credentials`, and the user has - * NOT re-typed the primary API key in the form (because `buildEditState` - * intentionally leaves raw credentials blank for security). + * NOT re-typed the primary API key in the form (because provider-owned edit + * hydration intentionally leaves raw credentials blank for security). * * `resolvePMCredentials` on the backend (`src/api/routers/pm-discovery.ts`) * resolves stored credentials when `projectId` is supplied, so this check diff --git a/web/src/components/projects/pm-wizard.tsx b/web/src/components/projects/pm-wizard.tsx index 559076a77..2c2c48dd6 100644 --- a/web/src/components/projects/pm-wizard.tsx +++ b/web/src/components/projects/pm-wizard.tsx @@ -22,7 +22,6 @@ import { // Plan 012/4: `WebhookStep` + `LinearWebhookInfoPanel` + supporting hooks // deleted; every provider owns its webhook UX via the manifest path. import { - buildEditState, createInitialState, isStep1Complete, type WizardAction, @@ -47,6 +46,18 @@ function confirmProviderSwitch(fromLabel: string, toLabel: string): boolean { return window.confirm(buildProviderSwitchConfirmationMessage(fromLabel, toLabel)); } +export function buildEditStateFromProviderWizard( + provider: string, + initialConfig: Record, + configuredKeys: ReadonlySet, +): Partial { + const manifestDef = getProviderWizard(provider); + if (!manifestDef) { + throw new Error(`No PM provider wizard registered for ${provider}`); + } + return manifestDef.buildEditState(initialConfig, configuredKeys); +} + export function ProviderPicker({ state, dispatch, @@ -261,7 +272,11 @@ export function PMWizard({ if (initializedRef.current) return; initializedRef.current = true; const configuredKeys = new Set(credentialsQuery.data.map((c) => c.envVarKey)); - const editState = buildEditState(initialProvider, initialConfig, configuredKeys); + const editState = buildEditStateFromProviderWizard( + initialProvider, + initialConfig, + configuredKeys, + ); dispatch({ type: 'INIT_EDIT', state: editState }); // Plan 011/4: open all steps up to a comfortable ceiling; actual // step count is provider-dependent (Trello 7, JIRA 8, Linear 7). From a5a47ed628db9de88c07de6d96955f2e4dc2faf2 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 14:38:19 +0200 Subject: [PATCH 08/15] fix(web): move webhook list normalization into providers (#1321) Co-authored-by: Cascade Bot --- tests/unit/web/jira-webhook-step.test.ts | 36 ++++++++++++- tests/unit/web/pm-wizard-state.test.ts | 52 ------------------- tests/unit/web/trello-webhook-step.test.ts | 36 ++++++++++++- .../pm-providers/jira/webhook-step.tsx | 20 ++++++- .../projects/pm-providers/jira/wizard.ts | 9 +--- .../pm-providers/trello/webhook-step.tsx | 20 ++++++- .../projects/pm-providers/trello/wizard.ts | 10 ++-- .../components/projects/pm-wizard-state.ts | 32 ------------ 8 files changed, 113 insertions(+), 102 deletions(-) diff --git a/tests/unit/web/jira-webhook-step.test.ts b/tests/unit/web/jira-webhook-step.test.ts index 779627323..0d89c6da5 100644 --- a/tests/unit/web/jira-webhook-step.test.ts +++ b/tests/unit/web/jira-webhook-step.test.ts @@ -11,7 +11,10 @@ import { createElement } from 'react'; import { renderToStaticMarkup } from 'react-dom/server'; import { describe, expect, it } from 'vitest'; -import { JiraWebhookAdapter } from '../../../web/src/components/projects/pm-providers/jira/webhook-step.js'; +import { + JiraWebhookAdapter, + normalizeJiraActiveWebhooks, +} from '../../../web/src/components/projects/pm-providers/jira/webhook-step.js'; import type { WizardState } from '../../../web/src/components/projects/pm-wizard-state.js'; function makeState(overrides: Partial = {}): WizardState { @@ -37,6 +40,24 @@ function makeProviderHooks(overrides: Record = {}): Record { + it('normalizes JIRA webhook list data from enabled to active', () => { + expect( + normalizeJiraActiveWebhooks({ + jira: [ + { id: 1, url: 'https://hook/jira', enabled: true }, + { id: 'wh-2', url: 'https://hook/jira-2', enabled: false }, + ], + }), + ).toEqual([ + { id: '1', url: 'https://hook/jira', active: true }, + { id: 'wh-2', url: 'https://hook/jira-2', active: false }, + ]); + }); + + it('normalizes missing JIRA webhook list data to an empty active list', () => { + expect(normalizeJiraActiveWebhooks(undefined)).toEqual([]); + }); + it('renders the shared WebhookUrlDisplayStep (URL + copy button)', () => { const html = renderToStaticMarkup( createElement(JiraWebhookAdapter, { @@ -88,6 +109,19 @@ describe('JiraWebhookAdapter', () => { expect(html).toContain('data-action="create-webhook"'); }); + it('renders the loading state while webhooks are loading', () => { + const html = renderToStaticMarkup( + createElement(JiraWebhookAdapter, { + state: makeState(), + dispatch: () => {}, + providerHooks: makeProviderHooks({ webhooksLoading: true }), + }), + ); + expect(html).toContain('data-state="loading"'); + expect(html).toContain('Loading webhooks'); + expect(html).not.toContain('No JIRA webhooks configured'); + }); + it('disables the Create button when callbackBaseUrl is empty', () => { const html = renderToStaticMarkup( createElement(JiraWebhookAdapter, { diff --git a/tests/unit/web/pm-wizard-state.test.ts b/tests/unit/web/pm-wizard-state.test.ts index 64ee0ee97..06505d63d 100644 --- a/tests/unit/web/pm-wizard-state.test.ts +++ b/tests/unit/web/pm-wizard-state.test.ts @@ -24,7 +24,6 @@ import { areCredentialsReady, buildLinearIntegrationConfig, createInitialState, - deriveActiveWebhooks, INITIAL_JIRA_LABELS, isStep1Complete, isStep2Complete, @@ -1041,54 +1040,3 @@ describe('buildLinearIntegrationConfig — save payload', () => { }); }); }); - -// ============================================================================ -// deriveActiveWebhooks -// ============================================================================ - -describe('deriveActiveWebhooks', () => { - it('maps Trello webhooks via callbackURL + active', () => { - const result = deriveActiveWebhooks('trello', { - trello: [ - { id: 1, callbackURL: 'https://hook/trello', active: true }, - { id: 2, callbackURL: 'https://hook/trello-2', active: false }, - ], - }); - - expect(result).toEqual([ - { id: '1', url: 'https://hook/trello', active: true }, - { id: '2', url: 'https://hook/trello-2', active: false }, - ]); - }); - - it('maps JIRA webhooks via url + enabled (renamed to active)', () => { - const result = deriveActiveWebhooks('jira', { - jira: [{ id: 'abc', url: 'https://hook/jira', enabled: true }], - }); - - expect(result).toEqual([{ id: 'abc', url: 'https://hook/jira', active: true }]); - }); - - it('returns empty array for Linear (manual webhook setup)', () => { - const result = deriveActiveWebhooks('linear', { - trello: [{ id: 1, callbackURL: 'irrelevant', active: true }], - jira: [{ id: 1, url: 'irrelevant', enabled: true }], - }); - - expect(result).toEqual([]); - }); - - it('returns empty array when webhooksData is undefined', () => { - expect(deriveActiveWebhooks('trello', undefined)).toEqual([]); - expect(deriveActiveWebhooks('jira', undefined)).toEqual([]); - expect(deriveActiveWebhooks('linear', undefined)).toEqual([]); - }); - - it('coerces numeric IDs to strings', () => { - const result = deriveActiveWebhooks('trello', { - trello: [{ id: 42, callbackURL: 'u', active: true }], - }); - expect(result[0].id).toBe('42'); - expect(typeof result[0].id).toBe('string'); - }); -}); diff --git a/tests/unit/web/trello-webhook-step.test.ts b/tests/unit/web/trello-webhook-step.test.ts index 73f248cf2..8a56a4fb4 100644 --- a/tests/unit/web/trello-webhook-step.test.ts +++ b/tests/unit/web/trello-webhook-step.test.ts @@ -14,7 +14,10 @@ import { createElement } from 'react'; import { renderToStaticMarkup } from 'react-dom/server'; import { describe, expect, it } from 'vitest'; -import { TrelloWebhookAdapter } from '../../../web/src/components/projects/pm-providers/trello/webhook-step.js'; +import { + normalizeTrelloActiveWebhooks, + TrelloWebhookAdapter, +} from '../../../web/src/components/projects/pm-providers/trello/webhook-step.js'; import type { WizardState } from '../../../web/src/components/projects/pm-wizard-state.js'; function makeState(overrides: Partial = {}): WizardState { @@ -40,6 +43,24 @@ function makeProviderHooks(overrides: Record = {}): Record { + it('normalizes Trello webhook list data from callbackURL to url', () => { + expect( + normalizeTrelloActiveWebhooks({ + trello: [ + { id: 1, callbackURL: 'https://hook/trello', active: true }, + { id: 2, callbackURL: 'https://hook/trello-2', active: false }, + ], + }), + ).toEqual([ + { id: '1', url: 'https://hook/trello', active: true }, + { id: '2', url: 'https://hook/trello-2', active: false }, + ]); + }); + + it('normalizes missing Trello webhook list data to an empty active list', () => { + expect(normalizeTrelloActiveWebhooks(undefined)).toEqual([]); + }); + it('renders the shared WebhookUrlDisplayStep (URL + copy button)', () => { const html = renderToStaticMarkup( createElement(TrelloWebhookAdapter, { @@ -91,6 +112,19 @@ describe('TrelloWebhookAdapter', () => { expect(html).toContain('data-action="create-webhook"'); }); + it('renders the loading state while webhooks are loading', () => { + const html = renderToStaticMarkup( + createElement(TrelloWebhookAdapter, { + state: makeState(), + dispatch: () => {}, + providerHooks: makeProviderHooks({ webhooksLoading: true }), + }), + ); + expect(html).toContain('data-state="loading"'); + expect(html).toContain('Loading webhooks'); + expect(html).not.toContain('No Trello webhooks configured'); + }); + it('disables the Create button when callbackBaseUrl is empty', () => { const html = renderToStaticMarkup( createElement(TrelloWebhookAdapter, { diff --git a/web/src/components/projects/pm-providers/jira/webhook-step.tsx b/web/src/components/projects/pm-providers/jira/webhook-step.tsx index bf560f372..df7abb5cf 100644 --- a/web/src/components/projects/pm-providers/jira/webhook-step.tsx +++ b/web/src/components/projects/pm-providers/jira/webhook-step.tsx @@ -20,12 +20,30 @@ import type { DataProps } from '@/lib/data-props.js'; import { WebhookUrlDisplayStep } from '../steps/webhook-url-display.js'; import type { ProviderWizardStepProps } from '../types.js'; -interface ActiveWebhook { +export interface ActiveWebhook { readonly id: string; readonly url: string; readonly active: boolean; } +export interface JiraWebhookListData { + readonly jira?: ReadonlyArray<{ + readonly id: string | number; + readonly url: string; + readonly enabled: boolean; + }>; +} + +export function normalizeJiraActiveWebhooks( + webhooksData: JiraWebhookListData | undefined, +): ActiveWebhook[] { + return (webhooksData?.jira ?? []).map((webhook) => ({ + id: String(webhook.id), + url: webhook.url, + active: webhook.enabled, + })); +} + interface JiraWebhookProviderHooks { readonly webhookUrl: string; readonly callbackBaseUrl: string; diff --git a/web/src/components/projects/pm-providers/jira/wizard.ts b/web/src/components/projects/pm-providers/jira/wizard.ts index b529e9b1d..92ffb4a38 100644 --- a/web/src/components/projects/pm-providers/jira/wizard.ts +++ b/web/src/components/projects/pm-providers/jira/wizard.ts @@ -20,7 +20,6 @@ import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import type { ReactElement } from 'react'; import { API_URL } from '@/lib/api.js'; import { trpc, trpcClient } from '@/lib/trpc.js'; -import { deriveActiveWebhooks } from '../../pm-wizard-state.js'; import { ContainerPickStep } from '../steps/container-pick.js'; import { CredentialsStep } from '../steps/credentials.js'; import { CustomFieldMappingStep } from '../steps/custom-field-mapping.js'; @@ -30,7 +29,7 @@ import type { ProviderWizardDefinition, ProviderWizardStepProps } from '../types import { jiraAuthMetadata, jiraCredentialPersistence } from './auth.js'; import { useJiraCustomFieldCreation, useJiraDiscovery } from './hooks.js'; import { IssueTypeMappingStep } from './issue-type-step.js'; -import { JiraWebhookAdapter } from './webhook-step.js'; +import { JiraWebhookAdapter, normalizeJiraActiveWebhooks } from './webhook-step.js'; // CASCADE stage keys that map to JIRA statuses (name-based, not id-based // — JIRA statuses are configured per project, name is the stable identity). @@ -358,11 +357,7 @@ export const jiraProviderWizard: ProviderWizardDefinition = { (typeof window !== 'undefined' ? window.location.origin.replace(':5173', ':3000') : ''); const webhooksQuery = useQuery(trpc.webhooks.list.queryOptions({ projectId: projectId ?? '' })); - const activeJiraWebhooks = deriveActiveWebhooks('jira', webhooksQuery.data) as Array<{ - id: string; - url: string; - active: boolean; - }>; + const activeJiraWebhooks = normalizeJiraActiveWebhooks(webhooksQuery.data); const createWebhookMutation = useMutation({ mutationFn: () => diff --git a/web/src/components/projects/pm-providers/trello/webhook-step.tsx b/web/src/components/projects/pm-providers/trello/webhook-step.tsx index 1d2953465..080ae13b8 100644 --- a/web/src/components/projects/pm-providers/trello/webhook-step.tsx +++ b/web/src/components/projects/pm-providers/trello/webhook-step.tsx @@ -20,12 +20,30 @@ import type { DataProps } from '@/lib/data-props.js'; import { WebhookUrlDisplayStep } from '../steps/webhook-url-display.js'; import type { ProviderWizardStepProps } from '../types.js'; -interface ActiveWebhook { +export interface ActiveWebhook { readonly id: string; readonly url: string; readonly active: boolean; } +export interface TrelloWebhookListData { + readonly trello?: ReadonlyArray<{ + readonly id: string | number; + readonly callbackURL: string; + readonly active: boolean; + }>; +} + +export function normalizeTrelloActiveWebhooks( + webhooksData: TrelloWebhookListData | undefined, +): ActiveWebhook[] { + return (webhooksData?.trello ?? []).map((webhook) => ({ + id: String(webhook.id), + url: webhook.callbackURL, + active: webhook.active, + })); +} + interface TrelloWebhookProviderHooks { readonly webhookUrl: string; readonly callbackBaseUrl: string; diff --git a/web/src/components/projects/pm-providers/trello/wizard.ts b/web/src/components/projects/pm-providers/trello/wizard.ts index 28a52d740..d7c4a0c3b 100644 --- a/web/src/components/projects/pm-providers/trello/wizard.ts +++ b/web/src/components/projects/pm-providers/trello/wizard.ts @@ -21,7 +21,7 @@ import type { ReactElement } from 'react'; import { useState } from 'react'; import { API_URL } from '@/lib/api.js'; import { trpc, trpcClient } from '@/lib/trpc.js'; -import { deriveActiveWebhooks, type WizardState } from '../../pm-wizard-state.js'; +import type { WizardState } from '../../pm-wizard-state.js'; import { ContainerPickStep } from '../steps/container-pick.js'; import { CustomFieldMappingStep } from '../steps/custom-field-mapping.js'; import { LabelMappingStep } from '../steps/label-mapping.js'; @@ -34,7 +34,7 @@ import { useTrelloLabelCreation, } from './hooks.js'; import { TrelloOAuthStep } from './oauth-step.js'; -import { TrelloWebhookAdapter } from './webhook-step.js'; +import { normalizeTrelloActiveWebhooks, TrelloWebhookAdapter } from './webhook-step.js'; // CASCADE stage keys that map to Trello lists (one list per stage). export const TRELLO_LIST_SLOTS = [ @@ -376,11 +376,7 @@ export const trelloProviderWizard: ProviderWizardDefinition = { (typeof window !== 'undefined' ? window.location.origin.replace(':5173', ':3000') : ''); const webhooksQuery = useQuery(trpc.webhooks.list.queryOptions({ projectId: projectId ?? '' })); - const activeTrelloWebhooks = deriveActiveWebhooks('trello', webhooksQuery.data) as Array<{ - id: string; - url: string; - active: boolean; - }>; + const activeTrelloWebhooks = normalizeTrelloActiveWebhooks(webhooksQuery.data); const createWebhookMutation = useMutation({ mutationFn: () => diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index a5a122a37..b5cbe66a2 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -432,35 +432,3 @@ export function buildLinearIntegrationConfig(state: WizardState): Record 0 ? { labels: state.linearLabels } : {}), }; } - -/** - * Map the provider's webhook listing into the shape expected by `WebhookStep`. - * Linear webhooks are configured manually outside the wizard; Trello/JIRA come - * from the corresponding API listing. - */ -export function deriveActiveWebhooks( - provider: Provider, - webhooksData: - | { - trello?: ReadonlyArray<{ id: string | number; callbackURL: string; active: boolean }>; - jira?: ReadonlyArray<{ id: string | number; url: string; enabled: boolean }>; - } - | undefined, -): Array<{ id: string; url: string; active: boolean }> { - if (provider === 'trello') { - return (webhooksData?.trello ?? []).map((w) => ({ - id: String(w.id), - url: w.callbackURL, - active: w.active, - })); - } - if (provider === 'jira') { - return (webhooksData?.jira ?? []).map((w) => ({ - id: String(w.id), - url: w.url, - active: w.enabled, - })); - } - // Linear: webhooks are configured manually - return []; -} From 1009be1a49e541734fc09431b7de47777e7a2121 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 14:56:31 +0200 Subject: [PATCH 09/15] fix(native-tools): render scalar prompt examples as CLI values (#1322) Co-authored-by: Cascade Bot --- CHANGELOG.md | 2 + src/backends/shared/nativeToolPrompts.ts | 83 ++++++++++++----- src/gadgets/README.md | 10 ++- .../backends/shared-nativeToolPrompts.test.ts | 89 +++++++++++++++++++ .../shared/promptExamplesGrammar.test.ts | 52 +++++++++++ 5 files changed, 213 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57416ad13..88b87cf4e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **Native-tool prompt examples for enum, scalar, number, and primitive-array flags now render as runnable CLI syntax instead of JSON string literals.** Agent-facing guidance now shows forms such as `cascade-tools scm create-pr-review --event APPROVE` and repeatable primitive arrays as `--labels bug --labels docs`, while object and array-of-object flags continue to render shell-quoted JSON payloads. See Trello card [l9Sira7y](https://trello.com/c/l9Sira7y/685-friction-tooling-low-create-pr-review-docs-show-quoted-enum-but-cli-requires-raw-enum). + - **`resolve-conflicts` agent no longer silently skips when GitHub's async mergeability computation hasn't resolved by the time the `pull_request` webhook is processed** (spec 020). `PRConflictDetectedTrigger` previously exhausted a 2×2s synchronous retry budget and silently discarded the event when `mergeable === null` — because GitHub never sends a follow-up webhook once mergeability resolves, the `resolve-conflicts` agent never fired. The trigger now returns `TriggerResult.deferredRecheck`, which causes the router to schedule a bare BullMQ delayed re-check job ~45s later via `scheduleCoalescedJob` (deduped per PR). The worker re-dispatches via the trigger registry to get fresh mergeability state. Multiple rapid webhooks for the same PR coalesce to a single re-check job. If mergeability is still `null` after the re-check fires, a Sentry event is captured under tag `mergeability_recheck_exhausted` and a WARN log is emitted — not a silent discard. Observed live on ucho/PR #329 (2026-05-07). See [spec 020](docs/specs/020-github-mergeability-deferred-recheck.md). ### Added diff --git a/src/backends/shared/nativeToolPrompts.ts b/src/backends/shared/nativeToolPrompts.ts index 0fd4fe7f8..40c397e44 100644 --- a/src/backends/shared/nativeToolPrompts.ts +++ b/src/backends/shared/nativeToolPrompts.ts @@ -14,6 +14,62 @@ You are operating in a native-tool environment, not a gadget/function-call envir - If you catch yourself composing a pseudo tool call in plain text, stop and use the real tool instead. - Trello, JIRA, and GitHub attachment URLs require backend authentication. NEVER curl, wget, or HTTP-fetch them — they return an authorization error. Work item images are pre-fetched and available either as images in your conversation context or as files under \`.cascade/context/images/\` — use whichever is present; never fetch the original URLs.`; +type PromptParamSchema = { + type: string; + required?: boolean; + default?: unknown; + description?: string; + options?: string[]; + items?: string; + aliases?: readonly string[]; + example?: unknown; +}; + +const SHELL_SAFE_VALUE_PATTERN = /^[A-Za-z0-9_./:@%+=,-]+$/; + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'"'"'`)}'`; +} + +function formatShellScalar(value: unknown): string { + const rendered = String(value); + if (rendered.length > 0 && SHELL_SAFE_VALUE_PATTERN.test(rendered)) { + return rendered; + } + return shellQuote(rendered); +} + +function formatJsonExample(value: unknown): string | undefined { + try { + return shellQuote(JSON.stringify(value)); + } catch { + // JSON.stringify throws on cyclic refs — never in our tool definitions, + // but be defensive so a malformed example never crashes prompt building. + return undefined; + } +} + +function formatExampleInvocation(key: string, schema: PromptParamSchema): string | undefined { + if (schema.example === undefined) return undefined; + + if (schema.type === 'boolean') { + return schema.example ? `--${key}` : `--no-${key}`; + } + + if (schema.type === 'object' || (schema.type === 'array' && schema.items === 'object')) { + const json = formatJsonExample(schema.example); + return json ? `--${key} ${json}` : undefined; + } + + if (schema.type === 'array') { + const examples = Array.isArray(schema.example) ? schema.example : [schema.example]; + if (examples.length === 0) return undefined; + return examples.map((value) => `--${key} ${formatShellScalar(value)}`).join(' '); + } + + return `--${key} ${formatShellScalar(schema.example)}`; +} + /** * Format a single CLI parameter for tool guidance documentation. * @@ -30,18 +86,7 @@ You are operating in a native-tool environment, not a gadget/function-call envir * - `type: 'object'` → renders as a single `''` blob. */ // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: parameter-type taxonomy -function formatParam( - key: string, - schema: { - type: string; - required?: boolean; - default?: unknown; - description?: string; - items?: string; - aliases?: readonly string[]; - example?: unknown; - }, -): string { +function formatParam(key: string, schema: PromptParamSchema): string { const aliasSuffix = (schema.aliases ?? []).map((a) => `|--${a}`).join(''); const flagHead = `--${key}${aliasSuffix}`; @@ -75,15 +120,9 @@ function formatParam( // per-flag example said exactly that. The synopsis renders the toggle form; // the example reinforces it concretely. if (schema.example !== undefined) { - if (schema.type === 'boolean') { - result += schema.example ? `\n # example: --${key}` : `\n # example: --no-${key}`; - } else { - try { - result += `\n # example: --${key} '${JSON.stringify(schema.example)}'`; - } catch { - // JSON.stringify throws on cyclic refs — never in our tool definitions, - // but be defensive so a malformed example never crashes prompt building. - } + const exampleInvocation = formatExampleInvocation(key, schema); + if (exampleInvocation) { + result += `\n # example: ${exampleInvocation}`; } } @@ -112,7 +151,7 @@ export function buildToolGuidance(tools: ToolManifest[]): string { guidance += `\`\`\`bash\n${tool.cliCommand}`; for (const [key, schema] of Object.entries(tool.parameters)) { - guidance += formatParam(key, schema as { type: string; required?: boolean }); + guidance += formatParam(key, schema as PromptParamSchema); } guidance += '\n```\n\n'; diff --git a/src/gadgets/README.md b/src/gadgets/README.md index d441540b4..79d5b65d5 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -63,12 +63,20 @@ cli: { A list of `{ params, comment, output? }` invocations. The first example that populates a given parameter becomes that parameter's **concrete example**, surfaced in three places: -- The agent's system prompt renders a one-line `# example: -- ''` under the flag (when the param is array-of-object / object). +- The agent's system prompt renders a one-line `# example: ...` under the flag. Object params and array-of-object params use a single shell-quoted JSON payload. Scalar, enum, number, and primitive-array params render as actual CLI syntax. - The `cascade-tools … --help` output lists every example as a runnable shell invocation under an `EXAMPLES` section. - JSON-parse failures include the example as the `expected` shape fragment in the structured error envelope. Write examples that a model could literally copy/paste. Use double-quoted JSON keys; do not rely on the agent to translate pseudo-JSON. +Enum examples must be raw CLI values, not JSON strings. For example, the review action should render as: + +```bash +cascade-tools scm create-pr-review --event APPROVE +``` + +not `--event '"APPROVE"'`. Primitive arrays should render as repeated flags (`--labels bug --labels docs`), while array-of-object examples like `comments` stay JSON (`--comments '[{"path":"src/x.ts","line":1,"body":"nit"}]'`). + ```ts examples: [ { diff --git a/tests/unit/backends/shared-nativeToolPrompts.test.ts b/tests/unit/backends/shared-nativeToolPrompts.test.ts index 83db9991e..2b73a439f 100644 --- a/tests/unit/backends/shared-nativeToolPrompts.test.ts +++ b/tests/unit/backends/shared-nativeToolPrompts.test.ts @@ -25,6 +25,8 @@ import { buildTaskPrompt, buildToolGuidance, } from '../../../src/backends/shared/nativeToolPrompts.js'; +import { createPRReviewDef } from '../../../src/gadgets/github/definitions.js'; +import { generateToolManifest } from '../../../src/gadgets/shared/manifestGenerator.js'; // ───────── helper ───────── function makeManifest(overrides: Partial = {}): ToolManifest { @@ -114,6 +116,63 @@ describe('buildToolGuidance', () => { }); }); + describe('formatParam — enum/scalar examples', () => { + it('renders enum examples as raw CLI values instead of JSON string literals', () => { + const result = buildToolGuidance([ + makeManifest({ + parameters: { + event: { + type: 'string', + options: ['APPROVE', 'REQUEST_CHANGES', 'COMMENT'], + required: true, + example: 'APPROVE', + }, + }, + }), + ]); + + expect(result).toContain('# example: --event APPROVE'); + expect(result).not.toContain(`--event '"APPROVE"'`); + expect(result).not.toContain(`--event '"REQUEST_CHANGES"'`); + }); + + it('renders number examples as raw CLI values', () => { + const result = buildToolGuidance([ + makeManifest({ + parameters: { + prNumber: { type: 'number', required: true, example: 42 }, + }, + }), + ]); + + expect(result).toContain('# example: --prNumber 42'); + expect(result).not.toContain(`--prNumber '42'`); + }); + + it('shell-quotes scalar string examples only when needed', () => { + const result = buildToolGuidance([ + makeManifest({ + parameters: { + body: { + type: 'string', + required: true, + example: 'LGTM after CI passes', + }, + }, + }), + ]); + + expect(result).toContain("# example: --body 'LGTM after CI passes'"); + }); + + it('renders CreatePRReview event guidance without JSON string quotes', () => { + const result = buildToolGuidance([generateToolManifest(createPRReviewDef)]); + + expect(result).toContain('# example: --event APPROVE'); + expect(result).not.toContain(`--event '"APPROVE"'`); + }); + }); + describe('formatParam — optional string param', () => { it('formats optional string param with brackets', () => { const result = buildToolGuidance([ @@ -156,6 +215,24 @@ describe('buildToolGuidance', () => { expect(result).toContain('[--labels (repeatable)]'); expect(result).not.toContain('--label '); // old bug }); + + it('renders primitive-array examples as repeated flags instead of a JSON blob', () => { + const result = buildToolGuidance([ + makeManifest({ + parameters: { + labels: { + type: 'array', + items: 'string', + required: false, + example: ['bug', 'docs'], + }, + }, + }), + ]); + + expect(result).toContain('# example: --labels bug --labels docs'); + expect(result).not.toContain(`--labels '["bug","docs"]'`); + }); }); describe('formatParam — array of object (items:"object") — JSON blob (spec 014)', () => { @@ -227,6 +304,18 @@ describe('buildToolGuidance', () => { expect(result).toContain("--config ''"); expect(result).not.toContain(''); // shouldn't leak the bare type }); + + it('renders object examples as a single JSON payload', () => { + const result = buildToolGuidance([ + makeManifest({ + parameters: { + config: { type: 'object', required: true, example: { mode: 'strict' } }, + }, + }), + ]); + + expect(result).toContain(`# example: --config '${JSON.stringify({ mode: 'strict' })}'`); + }); }); describe('formatParam — boolean param', () => { diff --git a/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts b/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts index 82abb156d..241f39415 100644 --- a/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts +++ b/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts @@ -21,6 +21,7 @@ import { describe, expect, it } from 'vitest'; import { buildToolGuidance } from '../../../../src/backends/shared/nativeToolPrompts.js'; import * as githubDefs from '../../../../src/gadgets/github/definitions.js'; +import { createPRReviewDef } from '../../../../src/gadgets/github/definitions.js'; import * as pmDefs from '../../../../src/gadgets/pm/definitions.js'; import * as sentryDefs from '../../../../src/gadgets/sentry/definitions.js'; import * as sessionDefs from '../../../../src/gadgets/session/definitions.js'; @@ -83,4 +84,55 @@ describe('prompt-rendered examples — CLI grammar correctness', () => { } }); } + + it('CreatePRReview renders enum examples as raw CLI values', () => { + const manifest = generateToolManifest(createPRReviewDef); + const rendered = buildToolGuidance([manifest]); + + expect(rendered).toContain('# example: --event APPROVE'); + expect(rendered).not.toContain(`--event '"APPROVE"'`); + expect(rendered).not.toContain(`--event '"REQUEST_CHANGES"'`); + expect(rendered).not.toContain(`--event '"COMMENT"'`); + }); + + it('CreatePRReview keeps array-of-object comments as JSON payload examples', () => { + const manifest = generateToolManifest(createPRReviewDef); + const rendered = buildToolGuidance([manifest]); + + expect(rendered).toContain( + `# example: --comments '${JSON.stringify([ + { + path: 'src/utils.ts', + line: 15, + body: 'This could cause a null pointer exception. Please add a null check.', + }, + ])}'`, + ); + expect(rendered).not.toContain('--comments (repeatable)'); + expect(rendered).not.toContain('--comments path'); + }); + + for (const def of allDefs) { + const enumFlagNames = Object.entries(def.parameters) + .filter(([, p]) => p.type === 'enum' && !p.gadgetOnly) + .map(([k]) => k); + + if (enumFlagNames.length === 0) continue; + + it(`${def.name}: enum flag examples use raw CLI grammar`, () => { + const manifest = generateToolManifest(def); + const rendered = buildToolGuidance([manifest]); + + for (const flagName of enumFlagNames) { + const options = + def.parameters[flagName]?.type === 'enum' ? def.parameters[flagName].options : []; + for (const option of options) { + expect( + rendered, + `${def.name}.${flagName} must not render --${flagName} '"${option}"'`, + ).not.toContain(`--${flagName} '"${option}"'`); + } + } + }); + } }); From 765240ab151e32af8e8144b69cd71cf005e20159 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 15:11:52 +0200 Subject: [PATCH 10/15] refactor(pm-wizard): keep PM config builders provider-owned (#1323) Co-authored-by: Cascade Bot --- tests/unit/web/pm-wizard-hooks.test.ts | 52 +++++++++---------- tests/unit/web/pm-wizard-state.test.ts | 13 +++-- tests/unit/web/wizard-alerts-slot.test.ts | 34 ++++++------ tests/unit/web/wizard-friction-slot.test.ts | 38 ++++++++------ .../projects/pm-providers/linear/wizard.ts | 8 ++- .../components/projects/pm-wizard-state.ts | 41 --------------- 6 files changed, 74 insertions(+), 112 deletions(-) diff --git a/tests/unit/web/pm-wizard-hooks.test.ts b/tests/unit/web/pm-wizard-hooks.test.ts index 35cbef470..f85e66ffe 100644 --- a/tests/unit/web/pm-wizard-hooks.test.ts +++ b/tests/unit/web/pm-wizard-hooks.test.ts @@ -1,7 +1,7 @@ /** * Unit tests for pure functions extracted in the pm-wizard-hooks refactor: * - runPerLabelCreations (batch label creator with per-item error handling) - * - buildTrelloIntegrationConfig / buildJiraIntegrationConfig (pure config builders) + * - provider-owned buildIntegrationConfig implementations */ import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -18,12 +18,7 @@ import { runPerLabelCreations, } from '../../../web/src/components/projects/pm-wizard-hooks.js'; import type { WizardState } from '../../../web/src/components/projects/pm-wizard-state.js'; -import { - buildJiraIntegrationConfig, - buildLinearIntegrationConfig, - buildTrelloIntegrationConfig, - createInitialState, -} from '../../../web/src/components/projects/pm-wizard-state.js'; +import { createInitialState } from '../../../web/src/components/projects/pm-wizard-state.js'; // ============================================================================ // Provider-owned credential metadata @@ -520,10 +515,10 @@ describe('metadata-driven save payloads', () => { }); // ============================================================================ -// buildTrelloIntegrationConfig +// trelloProviderWizard.buildIntegrationConfig // ============================================================================ -describe('buildTrelloIntegrationConfig', () => { +describe('trelloProviderWizard.buildIntegrationConfig', () => { function seed(overrides: Partial = {}): WizardState { return { ...createInitialState(), @@ -536,7 +531,7 @@ describe('buildTrelloIntegrationConfig', () => { } it('produces the expected config shape', () => { - const config = buildTrelloIntegrationConfig(seed()); + const config = trelloProviderWizard.buildIntegrationConfig(seed()); expect(config).toEqual({ boardId: 'board-abc', lists: { todo: 'list-1', done: 'list-2' }, @@ -545,17 +540,19 @@ describe('buildTrelloIntegrationConfig', () => { }); it('includes customFields when trelloCostFieldId is set', () => { - const config = buildTrelloIntegrationConfig(seed({ trelloCostFieldId: 'cf-cost' })); + const config = trelloProviderWizard.buildIntegrationConfig( + seed({ trelloCostFieldId: 'cf-cost' }), + ); expect(config.customFields).toEqual({ cost: 'cf-cost' }); }); it('omits customFields when trelloCostFieldId is empty', () => { - const config = buildTrelloIntegrationConfig(seed({ trelloCostFieldId: '' })); + const config = trelloProviderWizard.buildIntegrationConfig(seed({ trelloCostFieldId: '' })); expect(config).not.toHaveProperty('customFields'); }); it('passes through empty mappings', () => { - const config = buildTrelloIntegrationConfig( + const config = trelloProviderWizard.buildIntegrationConfig( seed({ trelloListMappings: {}, trelloLabelMappings: {} }), ); expect(config.lists).toEqual({}); @@ -564,10 +561,10 @@ describe('buildTrelloIntegrationConfig', () => { }); // ============================================================================ -// buildJiraIntegrationConfig +// jiraProviderWizard.buildIntegrationConfig // ============================================================================ -describe('buildJiraIntegrationConfig', () => { +describe('jiraProviderWizard.buildIntegrationConfig', () => { function seed(overrides: Partial = {}): WizardState { return { ...createInitialState(), @@ -581,7 +578,7 @@ describe('buildJiraIntegrationConfig', () => { } it('produces the expected config shape', () => { - const config = buildJiraIntegrationConfig(seed()); + const config = jiraProviderWizard.buildIntegrationConfig(seed()); expect(config).toEqual({ projectKey: 'PROJ', baseUrl: 'https://example.atlassian.net', @@ -591,39 +588,40 @@ describe('buildJiraIntegrationConfig', () => { }); it('includes issueTypes when jiraIssueTypes non-empty', () => { - const config = buildJiraIntegrationConfig( + const config = jiraProviderWizard.buildIntegrationConfig( seed({ jiraIssueTypes: { task: 'Task', subtask: 'Sub-task' } }), ); expect(config.issueTypes).toEqual({ task: 'Task', subtask: 'Sub-task' }); }); it('omits issueTypes when jiraIssueTypes is empty', () => { - const config = buildJiraIntegrationConfig(seed({ jiraIssueTypes: {} })); + const config = jiraProviderWizard.buildIntegrationConfig(seed({ jiraIssueTypes: {} })); expect(config).not.toHaveProperty('issueTypes'); }); it('omits labels when jiraLabels is empty', () => { - const config = buildJiraIntegrationConfig(seed({ jiraLabels: {} })); + const config = jiraProviderWizard.buildIntegrationConfig(seed({ jiraLabels: {} })); expect(config).not.toHaveProperty('labels'); }); it('includes customFields when jiraCostFieldId set', () => { - const config = buildJiraIntegrationConfig(seed({ jiraCostFieldId: 'customfield_10042' })); + const config = jiraProviderWizard.buildIntegrationConfig( + seed({ jiraCostFieldId: 'customfield_10042' }), + ); expect(config.customFields).toEqual({ cost: 'customfield_10042' }); }); it('omits customFields when jiraCostFieldId is empty', () => { - const config = buildJiraIntegrationConfig(seed({ jiraCostFieldId: '' })); + const config = jiraProviderWizard.buildIntegrationConfig(seed({ jiraCostFieldId: '' })); expect(config).not.toHaveProperty('customFields'); }); }); // ============================================================================ -// buildLinearIntegrationConfig (already tested in pm-wizard-state.test.ts; -// added here for cross-reference completeness) +// linearProviderWizard.buildIntegrationConfig // ============================================================================ -describe('buildLinearIntegrationConfig', () => { +describe('linearProviderWizard.buildIntegrationConfig', () => { function seed(overrides: Partial = {}): WizardState { return { ...createInitialState(), @@ -636,17 +634,17 @@ describe('buildLinearIntegrationConfig', () => { } it('produces the expected config shape', () => { - const config = buildLinearIntegrationConfig(seed()); + const config = linearProviderWizard.buildIntegrationConfig(seed()); expect(config).toEqual({ teamId: 'T1', statuses: { todo: 'S-TD' } }); }); it('includes projectId when linearProjectId is set', () => { - const config = buildLinearIntegrationConfig(seed({ linearProjectId: 'P1' })); + const config = linearProviderWizard.buildIntegrationConfig(seed({ linearProjectId: 'P1' })); expect(config.projectId).toBe('P1'); }); it('omits projectId when linearProjectId is empty', () => { - const config = buildLinearIntegrationConfig(seed({ linearProjectId: '' })); + const config = linearProviderWizard.buildIntegrationConfig(seed({ linearProjectId: '' })); expect(config).not.toHaveProperty('projectId'); }); }); diff --git a/tests/unit/web/pm-wizard-state.test.ts b/tests/unit/web/pm-wizard-state.test.ts index 06505d63d..7b19430d5 100644 --- a/tests/unit/web/pm-wizard-state.test.ts +++ b/tests/unit/web/pm-wizard-state.test.ts @@ -22,7 +22,6 @@ import type { } from '../../../web/src/components/projects/pm-wizard-state.js'; import { areCredentialsReady, - buildLinearIntegrationConfig, createInitialState, INITIAL_JIRA_LABELS, isStep1Complete, @@ -996,7 +995,7 @@ describe('Linear project scope — buildEditState hydration', () => { }); }); -describe('buildLinearIntegrationConfig — save payload', () => { +describe('linearProviderWizard.buildIntegrationConfig — save payload', () => { function seed(overrides: Partial = {}): WizardState { return { ...createInitialState(), @@ -1009,13 +1008,13 @@ describe('buildLinearIntegrationConfig — save payload', () => { } it('omits projectId when linearProjectId is empty', () => { - const config = buildLinearIntegrationConfig(seed({ linearProjectId: '' })); + const config = linearProviderWizard.buildIntegrationConfig(seed({ linearProjectId: '' })); expect(config).not.toHaveProperty('projectId'); expect(config.teamId).toBe('T1'); }); it('includes projectId when linearProjectId is set', () => { - const config = buildLinearIntegrationConfig(seed({ linearProjectId: 'P1' })); + const config = linearProviderWizard.buildIntegrationConfig(seed({ linearProjectId: 'P1' })); expect(config.projectId).toBe('P1'); expect(config.teamId).toBe('T1'); }); @@ -1023,14 +1022,14 @@ describe('buildLinearIntegrationConfig — save payload', () => { it('clearing a previously-set projectId yields a config without projectId', () => { // Simulate edit mode: start with projectId set, user clears, we save. const state = seed({ linearProjectId: '' }); // after clear - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect(config).not.toHaveProperty('projectId'); }); it('omits labels when linearLabels is empty; includes when populated', () => { - const bare = buildLinearIntegrationConfig(seed()); + const bare = linearProviderWizard.buildIntegrationConfig(seed()); expect(bare).not.toHaveProperty('labels'); - const withLabels = buildLinearIntegrationConfig( + const withLabels = linearProviderWizard.buildIntegrationConfig( // Linear labels are stored as UUIDs (the Linear API rejects names for // issueUpdate.labelIds). Wizard dropdowns populate from the team's labels. seed({ linearLabels: { processing: '11111111-1111-4111-8111-111111111111' } }), diff --git a/tests/unit/web/wizard-alerts-slot.test.ts b/tests/unit/web/wizard-alerts-slot.test.ts index 484896779..c8f1ad7bc 100644 --- a/tests/unit/web/wizard-alerts-slot.test.ts +++ b/tests/unit/web/wizard-alerts-slot.test.ts @@ -12,21 +12,19 @@ import { describe, expect, it } from 'vitest'; import { JIRA_LABEL_SLOTS, JIRA_STATUS_SLOTS, + jiraProviderWizard, } from '../../../web/src/components/projects/pm-providers/jira/wizard.js'; import { LINEAR_LABEL_SLOTS, LINEAR_STATUS_SLOTS, + linearProviderWizard, } from '../../../web/src/components/projects/pm-providers/linear/wizard.js'; import { TRELLO_LABEL_SLOTS, TRELLO_LIST_SLOTS, + trelloProviderWizard, } from '../../../web/src/components/projects/pm-providers/trello/wizard.js'; -import { - buildJiraIntegrationConfig, - buildLinearIntegrationConfig, - buildTrelloIntegrationConfig, - createInitialState, -} from '../../../web/src/components/projects/pm-wizard-state.js'; +import { createInitialState } from '../../../web/src/components/projects/pm-wizard-state.js'; // ============================================================================ // 1. Slot-array membership @@ -70,7 +68,7 @@ describe('alerts status slot round-trips through buildIntegrationConfig', () => ...createInitialState(), trelloListMappings: { alerts: 'list-id-alerts-123' }, }; - const config = buildTrelloIntegrationConfig(state); + const config = trelloProviderWizard.buildIntegrationConfig(state); expect((config.lists as Record).alerts).toBe('list-id-alerts-123'); }); @@ -79,7 +77,7 @@ describe('alerts status slot round-trips through buildIntegrationConfig', () => ...createInitialState(), jiraStatusMappings: { alerts: 'Alerts Status' }, }; - const config = buildJiraIntegrationConfig(state); + const config = jiraProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).alerts).toBe('Alerts Status'); }); @@ -88,7 +86,7 @@ describe('alerts status slot round-trips through buildIntegrationConfig', () => ...createInitialState(), linearStatusMappings: { alerts: 'state-uuid-abc' }, }; - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).alerts).toBe('state-uuid-abc'); }); }); @@ -99,7 +97,7 @@ describe('cascade-alert label slot round-trips through buildIntegrationConfig', ...createInitialState(), trelloLabelMappings: { 'cascade-alert': 'label-id-789' }, }; - const config = buildTrelloIntegrationConfig(state); + const config = trelloProviderWizard.buildIntegrationConfig(state); expect((config.labels as Record)['cascade-alert']).toBe('label-id-789'); }); @@ -108,7 +106,7 @@ describe('cascade-alert label slot round-trips through buildIntegrationConfig', ...createInitialState(), jiraLabels: { cascadeAlert: 'cascade-alert-label' }, }; - const config = buildJiraIntegrationConfig(state); + const config = jiraProviderWizard.buildIntegrationConfig(state); expect((config.labels as Record).cascadeAlert).toBe('cascade-alert-label'); }); @@ -117,7 +115,7 @@ describe('cascade-alert label slot round-trips through buildIntegrationConfig', ...createInitialState(), linearLabels: { cascadeAlert: 'label-uuid-xyz' }, }; - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect((config.labels as Record).cascadeAlert).toBe('label-uuid-xyz'); }); }); @@ -127,30 +125,30 @@ describe('cascade-alert label slot round-trips through buildIntegrationConfig', // ============================================================================ describe('alerts slot is optional — wizard does not require it', () => { - it('Trello: buildTrelloIntegrationConfig produces no alerts entry when not mapped', () => { + it('Trello: provider buildIntegrationConfig produces no alerts entry when not mapped', () => { const state = { ...createInitialState(), trelloListMappings: { todo: 'list-id-todo' }, }; - const config = buildTrelloIntegrationConfig(state); + const config = trelloProviderWizard.buildIntegrationConfig(state); expect((config.lists as Record).alerts).toBeUndefined(); }); - it('JIRA: buildJiraIntegrationConfig produces no alerts entry when not mapped', () => { + it('JIRA: provider buildIntegrationConfig produces no alerts entry when not mapped', () => { const state = { ...createInitialState(), jiraStatusMappings: { todo: 'To Do' }, }; - const config = buildJiraIntegrationConfig(state); + const config = jiraProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).alerts).toBeUndefined(); }); - it('Linear: buildLinearIntegrationConfig produces no alerts entry when not mapped', () => { + it('Linear: provider buildIntegrationConfig produces no alerts entry when not mapped', () => { const state = { ...createInitialState(), linearStatusMappings: { todo: 'state-uuid-todo' }, }; - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).alerts).toBeUndefined(); }); }); diff --git a/tests/unit/web/wizard-friction-slot.test.ts b/tests/unit/web/wizard-friction-slot.test.ts index 5877f44b0..d82099891 100644 --- a/tests/unit/web/wizard-friction-slot.test.ts +++ b/tests/unit/web/wizard-friction-slot.test.ts @@ -9,15 +9,19 @@ */ import { describe, expect, it } from 'vitest'; -import { JIRA_STATUS_SLOTS } from '../../../web/src/components/projects/pm-providers/jira/wizard.js'; -import { LINEAR_STATUS_SLOTS } from '../../../web/src/components/projects/pm-providers/linear/wizard.js'; -import { TRELLO_LIST_SLOTS } from '../../../web/src/components/projects/pm-providers/trello/wizard.js'; import { - buildJiraIntegrationConfig, - buildLinearIntegrationConfig, - buildTrelloIntegrationConfig, - createInitialState, -} from '../../../web/src/components/projects/pm-wizard-state.js'; + JIRA_STATUS_SLOTS, + jiraProviderWizard, +} from '../../../web/src/components/projects/pm-providers/jira/wizard.js'; +import { + LINEAR_STATUS_SLOTS, + linearProviderWizard, +} from '../../../web/src/components/projects/pm-providers/linear/wizard.js'; +import { + TRELLO_LIST_SLOTS, + trelloProviderWizard, +} from '../../../web/src/components/projects/pm-providers/trello/wizard.js'; +import { createInitialState } from '../../../web/src/components/projects/pm-wizard-state.js'; // ============================================================================ // 1. Slot-array membership @@ -47,7 +51,7 @@ describe('friction status slot round-trips through buildIntegrationConfig', () = ...createInitialState(), trelloListMappings: { friction: 'list-id-friction-123' }, }; - const config = buildTrelloIntegrationConfig(state); + const config = trelloProviderWizard.buildIntegrationConfig(state); expect((config.lists as Record).friction).toBe('list-id-friction-123'); }); @@ -56,7 +60,7 @@ describe('friction status slot round-trips through buildIntegrationConfig', () = ...createInitialState(), jiraStatusMappings: { friction: 'Friction Status' }, }; - const config = buildJiraIntegrationConfig(state); + const config = jiraProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).friction).toBe('Friction Status'); }); @@ -65,7 +69,7 @@ describe('friction status slot round-trips through buildIntegrationConfig', () = ...createInitialState(), linearStatusMappings: { friction: 'state-uuid-friction' }, }; - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).friction).toBe('state-uuid-friction'); }); }); @@ -75,30 +79,30 @@ describe('friction status slot round-trips through buildIntegrationConfig', () = // ============================================================================ describe('friction slot is optional — wizard does not require it', () => { - it('Trello: buildTrelloIntegrationConfig produces no friction entry when not mapped', () => { + it('Trello: provider buildIntegrationConfig produces no friction entry when not mapped', () => { const state = { ...createInitialState(), trelloListMappings: { todo: 'list-id-todo' }, }; - const config = buildTrelloIntegrationConfig(state); + const config = trelloProviderWizard.buildIntegrationConfig(state); expect((config.lists as Record).friction).toBeUndefined(); }); - it('JIRA: buildJiraIntegrationConfig produces no friction entry when not mapped', () => { + it('JIRA: provider buildIntegrationConfig produces no friction entry when not mapped', () => { const state = { ...createInitialState(), jiraStatusMappings: { todo: 'To Do' }, }; - const config = buildJiraIntegrationConfig(state); + const config = jiraProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).friction).toBeUndefined(); }); - it('Linear: buildLinearIntegrationConfig produces no friction entry when not mapped', () => { + it('Linear: provider buildIntegrationConfig produces no friction entry when not mapped', () => { const state = { ...createInitialState(), linearStatusMappings: { todo: 'state-uuid-todo' }, }; - const config = buildLinearIntegrationConfig(state); + const config = linearProviderWizard.buildIntegrationConfig(state); expect((config.statuses as Record).friction).toBeUndefined(); }); }); diff --git a/web/src/components/projects/pm-providers/linear/wizard.ts b/web/src/components/projects/pm-providers/linear/wizard.ts index 469328f3a..92983781c 100644 --- a/web/src/components/projects/pm-providers/linear/wizard.ts +++ b/web/src/components/projects/pm-providers/linear/wizard.ts @@ -23,7 +23,6 @@ import { useQuery } from '@tanstack/react-query'; import { type ReactElement, useState } from 'react'; import { API_URL } from '@/lib/api.js'; import { trpc } from '@/lib/trpc.js'; -import { buildLinearIntegrationConfig } from '../../pm-wizard-state.js'; import type { ProjectCredentialMeta } from '../../project-secret-field.js'; import { ContainerPickStep } from '../steps/container-pick.js'; import { CredentialsStep } from '../steps/credentials.js'; @@ -275,7 +274,12 @@ export const linearProviderWizard: ProviderWizardDefinition = { }, ], - buildIntegrationConfig: buildLinearIntegrationConfig, + buildIntegrationConfig: (state) => ({ + teamId: state.linearTeamId, + ...(state.linearProjectId ? { projectId: state.linearProjectId } : {}), + statuses: state.linearStatusMappings, + ...(Object.keys(state.linearLabels).length > 0 ? { labels: state.linearLabels } : {}), + }), buildEditState: (initialConfig, configuredKeys) => { const statuses = initialConfig.statuses as Record | undefined; diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index b5cbe66a2..e3fafe103 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -391,44 +391,3 @@ export function shouldUseStoredCredentials(state: WizardState): boolean { if (state.provider === 'jira') return !state.jiraApiToken; return !state.linearApiKey; } - -/** - * Build the Trello integration config payload from wizard state. - * Pure function so it can be unit-tested without the React runtime. - */ -export function buildTrelloIntegrationConfig(state: WizardState): Record { - return { - boardId: state.trelloBoardId, - lists: state.trelloListMappings, - labels: state.trelloLabelMappings, - ...(state.trelloCostFieldId ? { customFields: { cost: state.trelloCostFieldId } } : {}), - }; -} - -/** - * Build the JIRA integration config payload from wizard state. - * Pure function so it can be unit-tested without the React runtime. - */ -export function buildJiraIntegrationConfig(state: WizardState): Record { - return { - projectKey: state.jiraProjectKey, - baseUrl: state.jiraBaseUrl, - statuses: state.jiraStatusMappings, - ...(Object.keys(state.jiraIssueTypes).length > 0 ? { issueTypes: state.jiraIssueTypes } : {}), - ...(Object.keys(state.jiraLabels).length > 0 ? { labels: state.jiraLabels } : {}), - ...(state.jiraCostFieldId ? { customFields: { cost: state.jiraCostFieldId } } : {}), - }; -} - -/** - * Build the Linear integration config payload from wizard state. - * Pure function so it can be unit-tested without the React runtime. - */ -export function buildLinearIntegrationConfig(state: WizardState): Record { - return { - teamId: state.linearTeamId, - ...(state.linearProjectId ? { projectId: state.linearProjectId } : {}), - statuses: state.linearStatusMappings, - ...(Object.keys(state.linearLabels).length > 0 ? { labels: state.linearLabels } : {}), - }; -} From abe894599e34a0ff8a87ac0c2cca1d53fc3c43f0 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 15:42:35 +0200 Subject: [PATCH 11/15] refactor(web): delegate PM wizard provider actions (#1324) * refactor(web): delegate PM wizard provider actions * fix(web): remove duplicate credential cases from wizardReducer Remove the six provider-prefixed credential action cases (SET_TRELLO_API_KEY, SET_TRELLO_TOKEN, SET_JIRA_EMAIL, SET_JIRA_API_TOKEN, SET_JIRA_BASE_URL, SET_LINEAR_API_KEY) from the shared wizardReducer switch so they fall through to the default delegation path and are handled exclusively by the provider-owned reducers added in this PR. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Sonnet 4.6 --- tests/unit/web/pm-wizard-state.test.ts | 209 ++++------------ .../projects/pm-providers/jira/state.ts | 86 +++++++ .../projects/pm-providers/linear/state.ts | 67 ++++++ .../projects/pm-providers/trello/state.ts | 85 +++++++ .../components/projects/pm-wizard-state.ts | 224 ++---------------- 5 files changed, 297 insertions(+), 374 deletions(-) diff --git a/tests/unit/web/pm-wizard-state.test.ts b/tests/unit/web/pm-wizard-state.test.ts index 7b19430d5..211baaa67 100644 --- a/tests/unit/web/pm-wizard-state.test.ts +++ b/tests/unit/web/pm-wizard-state.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest'; import { createInitialJiraState, + jiraWizardReducer, INITIAL_JIRA_LABELS as PROVIDER_INITIAL_JIRA_LABELS, resetJiraProjectState, } from '../../../web/src/components/projects/pm-providers/jira/state.js'; @@ -8,12 +9,14 @@ import { jiraProviderWizard } from '../../../web/src/components/projects/pm-prov import { createInitialLinearState, INITIAL_LINEAR_LABELS, + linearWizardReducer, resetLinearTeamState, } from '../../../web/src/components/projects/pm-providers/linear/state.js'; import { linearProviderWizard } from '../../../web/src/components/projects/pm-providers/linear/wizard.js'; import { createInitialTrelloState, resetTrelloBoardState, + trelloWizardReducer, } from '../../../web/src/components/projects/pm-providers/trello/state.js'; import { trelloProviderWizard } from '../../../web/src/components/projects/pm-providers/trello/wizard.js'; import type { @@ -21,13 +24,9 @@ import type { WizardState, } from '../../../web/src/components/projects/pm-wizard-state.js'; import { - areCredentialsReady, createInitialState, INITIAL_JIRA_LABELS, isStep1Complete, - isStep2Complete, - isStep3Complete, - isStep4Complete, shouldUseStoredCredentials, wizardReducer, } from '../../../web/src/components/projects/pm-wizard-state.js'; @@ -149,6 +148,47 @@ describe('provider state slices', () => { linearProjects: [], }); }); + + it('handles Trello actions in the Trello state reducer', () => { + const state = { + ...createInitialState(), + trelloListMappings: { backlog: 'list-1' }, + }; + const next = trelloWizardReducer(state, { + type: 'SET_TRELLO_LIST_MAPPING', + key: 'todo', + value: 'list-2', + }); + expect(next.trelloListMappings).toEqual({ backlog: 'list-1', todo: 'list-2' }); + }); + + it('handles JIRA actions in the JIRA state reducer', () => { + const state = { + ...createInitialState(), + jiraIssueTypes: { task: 'Task' }, + }; + const next = jiraWizardReducer(state, { + type: 'SET_JIRA_ISSUE_TYPE', + key: 'subtask', + value: 'Sub-task', + }); + expect(next.jiraIssueTypes).toEqual({ task: 'Task', subtask: 'Sub-task' }); + }); + + it('handles Linear team reset in the Linear state reducer', () => { + const state = { + ...createInitialState(), + linearTeamId: 'team-1', + linearProjectId: 'project-1', + linearProjects: [{ id: 'project-1', name: 'Project 1', icon: null, color: null }], + linearStatusMappings: { todo: 'state-1' }, + }; + const next = linearWizardReducer(state, { type: 'SET_LINEAR_TEAM_ID', id: 'team-2' }); + expect(next.linearTeamId).toBe('team-2'); + expect(next.linearProjectId).toBe(''); + expect(next.linearProjects).toEqual([]); + expect(next.linearStatusMappings).toEqual({}); + }); }); // ============================================================================ @@ -559,167 +599,6 @@ describe('isStep1Complete', () => { }); }); -describe('isStep2Complete', () => { - it('returns true in edit mode when hasStoredCredentials is true (no raw creds needed)', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - isEditing: true, - hasStoredCredentials: true, - }; - expect(isStep2Complete(state)).toBe(true); - }); - - it('returns false when trello credentials missing', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - verificationResult: { provider: 'trello' as const, display: '@user' }, - }; - expect(isStep2Complete(state)).toBe(false); - }); - - it('returns false when trello creds present but no verification', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - trelloApiKey: 'my-api-key', - trelloToken: 'my-token', - }; - expect(isStep2Complete(state)).toBe(false); - }); - - it('returns true when trello creds present and verified', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - trelloApiKey: 'my-api-key', - trelloToken: 'my-token', - verificationResult: { provider: 'trello' as const, display: '@user (User)' }, - }; - expect(isStep2Complete(state)).toBe(true); - }); - - it('returns false when jira baseUrl missing even with creds', () => { - const state = { - ...createInitialState(), - provider: 'jira' as const, - jiraEmail: 'user@example.com', - jiraApiToken: 'my-token', - jiraBaseUrl: '', - verificationResult: { provider: 'jira' as const, display: 'User' }, - }; - expect(isStep2Complete(state)).toBe(false); - }); - - it('returns true when jira creds and baseUrl present and verified', () => { - const state = { - ...createInitialState(), - provider: 'jira' as const, - jiraEmail: 'user@example.com', - jiraApiToken: 'my-token', - jiraBaseUrl: 'https://myorg.atlassian.net', - verificationResult: { provider: 'jira' as const, display: 'User (user@example.com)' }, - }; - expect(isStep2Complete(state)).toBe(true); - }); -}); - -describe('isStep3Complete', () => { - it('returns true for trello when boardId set', () => { - const state = { ...createInitialState(), provider: 'trello' as const, trelloBoardId: 'b1' }; - expect(isStep3Complete(state)).toBe(true); - }); - - it('returns false for trello when boardId empty', () => { - const state = { ...createInitialState(), provider: 'trello' as const }; - expect(isStep3Complete(state)).toBe(false); - }); - - it('returns true for jira when projectKey set', () => { - const state = { ...createInitialState(), provider: 'jira' as const, jiraProjectKey: 'PROJ' }; - expect(isStep3Complete(state)).toBe(true); - }); - - it('returns false for jira when projectKey empty', () => { - const state = { ...createInitialState(), provider: 'jira' as const }; - expect(isStep3Complete(state)).toBe(false); - }); -}); - -describe('isStep4Complete', () => { - it('returns true for trello when any list mapping set', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - trelloListMappings: { todo: 'list-1' }, - }; - expect(isStep4Complete(state)).toBe(true); - }); - - it('returns false for trello when no list mappings', () => { - const state = { ...createInitialState(), provider: 'trello' as const }; - expect(isStep4Complete(state)).toBe(false); - }); - - it('returns true for jira when any status mapping set', () => { - const state = { - ...createInitialState(), - provider: 'jira' as const, - jiraStatusMappings: { todo: 'To Do' }, - }; - expect(isStep4Complete(state)).toBe(true); - }); - - it('returns false for jira when no status mappings', () => { - const state = { ...createInitialState(), provider: 'jira' as const }; - expect(isStep4Complete(state)).toBe(false); - }); -}); - -describe('areCredentialsReady', () => { - it('returns true for trello when both credentials set', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - trelloApiKey: 'my-api-key', - trelloToken: 'my-token', - }; - expect(areCredentialsReady(state)).toBe(true); - }); - - it('returns false for trello when one credential missing', () => { - const state = { - ...createInitialState(), - provider: 'trello' as const, - trelloApiKey: 'my-api-key', - }; - expect(areCredentialsReady(state)).toBe(false); - }); - - it('returns true for jira when email, api token, and baseUrl set', () => { - const state = { - ...createInitialState(), - provider: 'jira' as const, - jiraEmail: 'user@example.com', - jiraApiToken: 'my-token', - jiraBaseUrl: 'https://myorg.atlassian.net', - }; - expect(areCredentialsReady(state)).toBe(true); - }); - - it('returns false for jira when baseUrl missing', () => { - const state = { - ...createInitialState(), - provider: 'jira' as const, - jiraEmail: 'user@example.com', - jiraApiToken: 'my-token', - jiraBaseUrl: '', - }; - expect(areCredentialsReady(state)).toBe(false); - }); -}); - describe('shouldUseStoredCredentials', () => { // When editing an existing integration, the form does NOT pre-fill // the API key for security — `hasStoredCredentials` is flipped true diff --git a/web/src/components/projects/pm-providers/jira/state.ts b/web/src/components/projects/pm-providers/jira/state.ts index d2c339e51..08417f28d 100644 --- a/web/src/components/projects/pm-providers/jira/state.ts +++ b/web/src/components/projects/pm-providers/jira/state.ts @@ -30,6 +30,24 @@ export interface JiraWizardStateSlice { jiraCostFieldId: string; } +interface VerificationState { + verificationResult: { provider: string; display: string } | null; + verifyError: string | null; +} + +export type JiraWizardAction = + | { type: 'SET_JIRA_EMAIL'; value: string } + | { type: 'SET_JIRA_API_TOKEN'; value: string } + | { type: 'SET_JIRA_BASE_URL'; url: string } + | { type: 'SET_JIRA_PROJECTS'; projects: JiraProjectOption[] } + | { type: 'SET_JIRA_PROJECT_KEY'; key: string } + | { type: 'SET_JIRA_PROJECT_DETAILS'; details: JiraProjectDetails | null } + | { type: 'SET_JIRA_STATUS_MAPPING'; key: string; value: string } + | { type: 'SET_JIRA_ISSUE_TYPE'; key: string; value: string } + | { type: 'SET_JIRA_LABEL'; key: string; value: string } + | { type: 'SET_JIRA_COST_FIELD'; id: string } + | { type: 'ADD_JIRA_PROJECT_CUSTOM_FIELD'; field: { id: string; name: string; custom: boolean } }; + export function createInitialJiraState(): JiraWizardStateSlice { return { jiraEmail: '', @@ -45,6 +63,74 @@ export function createInitialJiraState(): JiraWizardStateSlice { }; } +export function isJiraWizardAction(action: { type: string }): action is JiraWizardAction { + return action.type.includes('JIRA'); +} + +export function jiraWizardReducer( + state: T, + action: JiraWizardAction, +): T { + switch (action.type) { + case 'SET_JIRA_EMAIL': + return { + ...state, + jiraEmail: action.value, + verificationResult: null, + verifyError: null, + }; + case 'SET_JIRA_API_TOKEN': + return { + ...state, + jiraApiToken: action.value, + verificationResult: null, + verifyError: null, + }; + case 'SET_JIRA_BASE_URL': + return { + ...state, + jiraBaseUrl: action.url, + verificationResult: null, + verifyError: null, + }; + case 'SET_JIRA_PROJECTS': + return { ...state, jiraProjects: action.projects }; + case 'SET_JIRA_PROJECT_KEY': + return { + ...state, + ...resetJiraProjectState(action.key), + }; + case 'SET_JIRA_PROJECT_DETAILS': + return { ...state, jiraProjectDetails: action.details }; + case 'SET_JIRA_STATUS_MAPPING': + return { + ...state, + jiraStatusMappings: { ...state.jiraStatusMappings, [action.key]: action.value }, + }; + case 'SET_JIRA_ISSUE_TYPE': + return { + ...state, + jiraIssueTypes: { ...state.jiraIssueTypes, [action.key]: action.value }, + }; + case 'SET_JIRA_LABEL': + return { + ...state, + jiraLabels: { ...state.jiraLabels, [action.key]: action.value }, + }; + case 'SET_JIRA_COST_FIELD': + return { ...state, jiraCostFieldId: action.id }; + case 'ADD_JIRA_PROJECT_CUSTOM_FIELD': + if (!state.jiraProjectDetails) return state; + return { + ...state, + jiraProjectDetails: { + ...state.jiraProjectDetails, + fields: [...state.jiraProjectDetails.fields, action.field], + }, + }; + } +} + export function resetJiraProjectState( jiraProjectKey: string, ): Pick< diff --git a/web/src/components/projects/pm-providers/linear/state.ts b/web/src/components/projects/pm-providers/linear/state.ts index 006ffd36a..a81ea9981 100644 --- a/web/src/components/projects/pm-providers/linear/state.ts +++ b/web/src/components/projects/pm-providers/linear/state.ts @@ -35,6 +35,22 @@ export interface LinearWizardStateSlice { linearLabels: Record; } +interface VerificationState { + verificationResult: { provider: string; display: string } | null; + verifyError: string | null; +} + +export type LinearWizardAction = + | { type: 'SET_LINEAR_API_KEY'; value: string } + | { type: 'SET_LINEAR_TEAMS'; teams: LinearTeamOption[] } + | { type: 'SET_LINEAR_TEAM_ID'; id: string } + | { type: 'SET_LINEAR_TEAM_DETAILS'; details: LinearTeamDetails | null } + | { type: 'SET_LINEAR_PROJECTS'; projects: LinearProjectOption[] } + | { type: 'SET_LINEAR_PROJECT_ID'; value: string } + | { type: 'SET_LINEAR_STATUS_MAPPING'; key: string; value: string } + | { type: 'SET_LINEAR_LABEL'; key: string; value: string } + | { type: 'ADD_LINEAR_TEAM_LABEL'; label: { id: string; name: string; color: string } }; + export function createInitialLinearState(): LinearWizardStateSlice { return { linearApiKey: '', @@ -48,6 +64,57 @@ export function createInitialLinearState(): LinearWizardStateSlice { }; } +export function isLinearWizardAction(action: { type: string }): action is LinearWizardAction { + return action.type.includes('LINEAR'); +} + +export function linearWizardReducer( + state: T, + action: LinearWizardAction, +): T { + switch (action.type) { + case 'SET_LINEAR_API_KEY': + return { + ...state, + linearApiKey: action.value, + verificationResult: null, + verifyError: null, + }; + case 'SET_LINEAR_TEAMS': + return { ...state, linearTeams: action.teams }; + case 'SET_LINEAR_TEAM_ID': + return { + ...state, + ...resetLinearTeamState(action.id), + }; + case 'SET_LINEAR_PROJECTS': + return { ...state, linearProjects: action.projects }; + case 'SET_LINEAR_PROJECT_ID': + return { ...state, linearProjectId: action.value }; + case 'SET_LINEAR_TEAM_DETAILS': + return { ...state, linearTeamDetails: action.details }; + case 'SET_LINEAR_STATUS_MAPPING': + return { + ...state, + linearStatusMappings: { ...state.linearStatusMappings, [action.key]: action.value }, + }; + case 'SET_LINEAR_LABEL': + return { + ...state, + linearLabels: { ...state.linearLabels, [action.key]: action.value }, + }; + case 'ADD_LINEAR_TEAM_LABEL': + if (!state.linearTeamDetails) return state; + return { + ...state, + linearTeamDetails: { + ...state.linearTeamDetails, + labels: [...state.linearTeamDetails.labels, action.label], + }, + }; + } +} + export function resetLinearTeamState( linearTeamId: string, ): Pick< diff --git a/web/src/components/projects/pm-providers/trello/state.ts b/web/src/components/projects/pm-providers/trello/state.ts index 336cdbf1f..6a31aa34e 100644 --- a/web/src/components/projects/pm-providers/trello/state.ts +++ b/web/src/components/projects/pm-providers/trello/state.ts @@ -21,6 +21,26 @@ export interface TrelloWizardStateSlice { trelloCostFieldId: string; } +interface VerificationState { + verificationResult: { provider: string; display: string } | null; + verifyError: string | null; +} + +export type TrelloWizardAction = + | { type: 'SET_TRELLO_API_KEY'; value: string } + | { type: 'SET_TRELLO_TOKEN'; value: string } + | { type: 'SET_TRELLO_BOARDS'; boards: TrelloBoardOption[] } + | { type: 'SET_TRELLO_BOARD_ID'; id: string } + | { type: 'SET_TRELLO_BOARD_DETAILS'; details: TrelloBoardDetails | null } + | { type: 'SET_TRELLO_LIST_MAPPING'; key: string; value: string } + | { type: 'SET_TRELLO_LABEL_MAPPING'; key: string; value: string } + | { type: 'SET_TRELLO_COST_FIELD'; id: string } + | { type: 'ADD_TRELLO_BOARD_LABEL'; label: { id: string; name: string; color: string } } + | { + type: 'ADD_TRELLO_BOARD_CUSTOM_FIELD'; + customField: { id: string; name: string; type: string }; + }; + export function createInitialTrelloState(): TrelloWizardStateSlice { return { trelloApiKey: '', @@ -34,6 +54,71 @@ export function createInitialTrelloState(): TrelloWizardStateSlice { }; } +export function isTrelloWizardAction(action: { type: string }): action is TrelloWizardAction { + return action.type.includes('TRELLO'); +} + +export function trelloWizardReducer( + state: T, + action: TrelloWizardAction, +): T { + switch (action.type) { + case 'SET_TRELLO_API_KEY': + return { + ...state, + trelloApiKey: action.value, + verificationResult: null, + verifyError: null, + }; + case 'SET_TRELLO_TOKEN': + return { + ...state, + trelloToken: action.value, + verificationResult: null, + verifyError: null, + }; + case 'SET_TRELLO_BOARDS': + return { ...state, trelloBoards: action.boards }; + case 'SET_TRELLO_BOARD_ID': + return { + ...state, + ...resetTrelloBoardState(action.id), + }; + case 'SET_TRELLO_BOARD_DETAILS': + return { ...state, trelloBoardDetails: action.details }; + case 'SET_TRELLO_LIST_MAPPING': + return { + ...state, + trelloListMappings: { ...state.trelloListMappings, [action.key]: action.value }, + }; + case 'SET_TRELLO_LABEL_MAPPING': + return { + ...state, + trelloLabelMappings: { ...state.trelloLabelMappings, [action.key]: action.value }, + }; + case 'SET_TRELLO_COST_FIELD': + return { ...state, trelloCostFieldId: action.id }; + case 'ADD_TRELLO_BOARD_LABEL': + if (!state.trelloBoardDetails) return state; + return { + ...state, + trelloBoardDetails: { + ...state.trelloBoardDetails, + labels: [...state.trelloBoardDetails.labels, action.label], + }, + }; + case 'ADD_TRELLO_BOARD_CUSTOM_FIELD': + if (!state.trelloBoardDetails) return state; + return { + ...state, + trelloBoardDetails: { + ...state.trelloBoardDetails, + customFields: [...state.trelloBoardDetails.customFields, action.customField], + }, + }; + } +} + export function resetTrelloBoardState( trelloBoardId: string, ): Pick< diff --git a/web/src/components/projects/pm-wizard-state.ts b/web/src/components/projects/pm-wizard-state.ts index e3fafe103..78a8c99a7 100644 --- a/web/src/components/projects/pm-wizard-state.ts +++ b/web/src/components/projects/pm-wizard-state.ts @@ -6,23 +6,29 @@ import type { Reducer } from 'react'; import { createInitialJiraState, INITIAL_JIRA_LABELS, + isJiraWizardAction, type JiraProjectDetails, type JiraProjectOption, - resetJiraProjectState, + type JiraWizardAction, + jiraWizardReducer, } from './pm-providers/jira/state.js'; import { createInitialLinearState, INITIAL_LINEAR_LABELS, + isLinearWizardAction, type LinearProjectOption, type LinearTeamDetails, type LinearTeamOption, - resetLinearTeamState, + type LinearWizardAction, + linearWizardReducer, } from './pm-providers/linear/state.js'; import { createInitialTrelloState, - resetTrelloBoardState, + isTrelloWizardAction, type TrelloBoardDetails, type TrelloBoardOption, + type TrelloWizardAction, + trelloWizardReducer, } from './pm-providers/trello/state.js'; export type { @@ -105,45 +111,15 @@ export interface WizardState { export type WizardAction = | { type: 'SET_PROVIDER'; provider: Provider } - | { type: 'SET_TRELLO_API_KEY'; value: string } - | { type: 'SET_TRELLO_TOKEN'; value: string } - | { type: 'SET_JIRA_EMAIL'; value: string } - | { type: 'SET_JIRA_API_TOKEN'; value: string } - | { type: 'SET_JIRA_BASE_URL'; url: string } - | { type: 'SET_LINEAR_API_KEY'; value: string } | { type: 'SET_VERIFICATION'; result: { provider: Provider; display: string } | null; error?: string | null; } - | { type: 'SET_TRELLO_BOARDS'; boards: TrelloBoardOption[] } - | { type: 'SET_TRELLO_BOARD_ID'; id: string } - | { type: 'SET_JIRA_PROJECTS'; projects: JiraProjectOption[] } - | { type: 'SET_JIRA_PROJECT_KEY'; key: string } - | { type: 'SET_LINEAR_TEAMS'; teams: LinearTeamOption[] } - | { type: 'SET_LINEAR_TEAM_ID'; id: string } - | { type: 'SET_LINEAR_TEAM_DETAILS'; details: LinearTeamDetails | null } - | { type: 'SET_LINEAR_PROJECTS'; projects: LinearProjectOption[] } - | { type: 'SET_LINEAR_PROJECT_ID'; value: string } - | { type: 'SET_TRELLO_BOARD_DETAILS'; details: TrelloBoardDetails | null } - | { type: 'SET_JIRA_PROJECT_DETAILS'; details: JiraProjectDetails | null } - | { type: 'SET_TRELLO_LIST_MAPPING'; key: string; value: string } - | { type: 'SET_TRELLO_LABEL_MAPPING'; key: string; value: string } - | { type: 'SET_TRELLO_COST_FIELD'; id: string } - | { type: 'SET_JIRA_STATUS_MAPPING'; key: string; value: string } - | { type: 'SET_JIRA_ISSUE_TYPE'; key: string; value: string } - | { type: 'SET_JIRA_LABEL'; key: string; value: string } - | { type: 'SET_JIRA_COST_FIELD'; id: string } - | { type: 'SET_LINEAR_STATUS_MAPPING'; key: string; value: string } - | { type: 'SET_LINEAR_LABEL'; key: string; value: string } | { type: 'INIT_EDIT'; state: Partial } - | { type: 'ADD_TRELLO_BOARD_LABEL'; label: { id: string; name: string; color: string } } - | { type: 'ADD_LINEAR_TEAM_LABEL'; label: { id: string; name: string; color: string } } - | { - type: 'ADD_TRELLO_BOARD_CUSTOM_FIELD'; - customField: { id: string; name: string; type: string }; - } - | { type: 'ADD_JIRA_PROJECT_CUSTOM_FIELD'; field: { id: string; name: string; custom: boolean } }; + | TrelloWizardAction + | JiraWizardAction + | LinearWizardAction; // ============================================================================ // Initial state and constants @@ -177,158 +153,18 @@ export const wizardReducer: Reducer = (state, action) isEditing: state.isEditing, previousProvider: state.previousProvider, }; - case 'SET_TRELLO_API_KEY': - return { - ...state, - trelloApiKey: action.value, - verificationResult: null, - verifyError: null, - }; - case 'SET_TRELLO_TOKEN': - return { - ...state, - trelloToken: action.value, - verificationResult: null, - verifyError: null, - }; - case 'SET_JIRA_EMAIL': - return { - ...state, - jiraEmail: action.value, - verificationResult: null, - verifyError: null, - }; - case 'SET_JIRA_API_TOKEN': - return { - ...state, - jiraApiToken: action.value, - verificationResult: null, - verifyError: null, - }; - case 'SET_JIRA_BASE_URL': - return { ...state, jiraBaseUrl: action.url, verificationResult: null, verifyError: null }; - case 'SET_LINEAR_API_KEY': - return { - ...state, - linearApiKey: action.value, - verificationResult: null, - verifyError: null, - }; case 'SET_VERIFICATION': return { ...state, verificationResult: action.result, verifyError: action.error ?? null }; - case 'SET_TRELLO_BOARDS': - return { ...state, trelloBoards: action.boards }; - case 'SET_TRELLO_BOARD_ID': - return { - ...state, - ...resetTrelloBoardState(action.id), - }; - case 'SET_JIRA_PROJECTS': - return { ...state, jiraProjects: action.projects }; - case 'SET_JIRA_PROJECT_KEY': - return { - ...state, - ...resetJiraProjectState(action.key), - }; - case 'SET_LINEAR_TEAMS': - return { ...state, linearTeams: action.teams }; - case 'SET_LINEAR_TEAM_ID': - return { - ...state, - ...resetLinearTeamState(action.id), - }; - case 'SET_LINEAR_PROJECTS': - return { ...state, linearProjects: action.projects }; - case 'SET_LINEAR_PROJECT_ID': - return { ...state, linearProjectId: action.value }; - case 'SET_LINEAR_TEAM_DETAILS': - return { ...state, linearTeamDetails: action.details }; - case 'SET_TRELLO_BOARD_DETAILS': - return { ...state, trelloBoardDetails: action.details }; - case 'SET_JIRA_PROJECT_DETAILS': - return { ...state, jiraProjectDetails: action.details }; - case 'SET_TRELLO_LIST_MAPPING': - return { - ...state, - trelloListMappings: { ...state.trelloListMappings, [action.key]: action.value }, - }; - case 'SET_TRELLO_LABEL_MAPPING': - return { - ...state, - trelloLabelMappings: { ...state.trelloLabelMappings, [action.key]: action.value }, - }; - case 'SET_TRELLO_COST_FIELD': - return { ...state, trelloCostFieldId: action.id }; - case 'SET_JIRA_STATUS_MAPPING': - return { - ...state, - jiraStatusMappings: { ...state.jiraStatusMappings, [action.key]: action.value }, - }; - case 'SET_JIRA_ISSUE_TYPE': - return { - ...state, - jiraIssueTypes: { ...state.jiraIssueTypes, [action.key]: action.value }, - }; - case 'SET_JIRA_LABEL': - return { - ...state, - jiraLabels: { ...state.jiraLabels, [action.key]: action.value }, - }; - case 'SET_JIRA_COST_FIELD': - return { ...state, jiraCostFieldId: action.id }; - case 'SET_LINEAR_STATUS_MAPPING': - return { - ...state, - linearStatusMappings: { ...state.linearStatusMappings, [action.key]: action.value }, - }; - case 'SET_LINEAR_LABEL': - return { - ...state, - linearLabels: { ...state.linearLabels, [action.key]: action.value }, - }; case 'INIT_EDIT': { const merged = { ...state, ...action.state, isEditing: true }; // Snapshot the loaded provider so a later SET_PROVIDER knows what to clean up. merged.previousProvider = merged.provider; return merged; } - case 'ADD_TRELLO_BOARD_LABEL': - if (!state.trelloBoardDetails) return state; - return { - ...state, - trelloBoardDetails: { - ...state.trelloBoardDetails, - labels: [...state.trelloBoardDetails.labels, action.label], - }, - }; - case 'ADD_LINEAR_TEAM_LABEL': - if (!state.linearTeamDetails) return state; - return { - ...state, - linearTeamDetails: { - ...state.linearTeamDetails, - labels: [...state.linearTeamDetails.labels, action.label], - }, - }; - case 'ADD_TRELLO_BOARD_CUSTOM_FIELD': - if (!state.trelloBoardDetails) return state; - return { - ...state, - trelloBoardDetails: { - ...state.trelloBoardDetails, - customFields: [...state.trelloBoardDetails.customFields, action.customField], - }, - }; - case 'ADD_JIRA_PROJECT_CUSTOM_FIELD': - if (!state.jiraProjectDetails) return state; - return { - ...state, - jiraProjectDetails: { - ...state.jiraProjectDetails, - fields: [...state.jiraProjectDetails.fields, action.field], - }, - }; default: + if (isTrelloWizardAction(action)) return trelloWizardReducer(state, action); + if (isJiraWizardAction(action)) return jiraWizardReducer(state, action); + if (isLinearWizardAction(action)) return linearWizardReducer(state, action); return state; } }; @@ -341,36 +177,6 @@ export function isStep1Complete(state: WizardState): boolean { return !!state.provider; } -export function isStep2Complete(state: WizardState): boolean { - if (state.isEditing && state.hasStoredCredentials) return true; - const credsReady = - state.provider === 'trello' - ? !!(state.trelloApiKey && state.trelloToken) - : state.provider === 'jira' - ? !!(state.jiraEmail && state.jiraApiToken && state.jiraBaseUrl) - : !!state.linearApiKey; - return credsReady && !!state.verificationResult; -} - -export function isStep3Complete(state: WizardState): boolean { - if (state.provider === 'trello') return !!state.trelloBoardId; - if (state.provider === 'jira') return !!state.jiraProjectKey; - return !!state.linearTeamId; -} - -export function isStep4Complete(state: WizardState): boolean { - if (state.provider === 'trello') return Object.keys(state.trelloListMappings).length > 0; - if (state.provider === 'jira') return Object.keys(state.jiraStatusMappings).length > 0; - return Object.keys(state.linearStatusMappings).length > 0; -} - -export function areCredentialsReady(state: WizardState): boolean { - if (state.provider === 'trello') return !!(state.trelloApiKey && state.trelloToken); - if (state.provider === 'jira') - return !!(state.jiraEmail && state.jiraApiToken && state.jiraBaseUrl); - return !!state.linearApiKey; -} - /** * Returns `true` when a wizard mutation (verify, createLabel, createCustomField) * should pass `projectId` to the backend — meaning: edit mode is active, the From a612a6cb9e64bb6b41d4f180695df162828ea495 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 16:11:42 +0200 Subject: [PATCH 12/15] docs(pm): document frontend provider boundary (#1325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(pm): document frontend provider boundary * fix(test): add SHA-256 hash guard for shared PM wizard orchestration files The existence-only check in new-provider-surface.test.ts was not enough to enforce the invariant documented in the README and architecture docs — a future provider PR could edit pm-wizard.tsx/pm-wizard-hooks.ts/ pm-wizard-common-steps.tsx and CI would still pass as long as those files remained non-empty. Add GUARDED_WIZARD_FILE_HASHES with pinned SHA-256 hashes for the three shared wizard orchestration files. The new it.each assertion computes the actual hash at test time and fails with a diagnostic message when a file is modified, matching the "guarded shared surface" claim in the docs. Legitimate infrastructure edits to those files (e.g. adding a new StandardStepKind) must update the corresponding hash and include a commit justification. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: Cascade Bot Co-authored-by: Claude Sonnet 4.6 --- docs/architecture/06-integration-layer.md | 2 + src/integrations/README.md | 34 +++++--- .../integrations/new-provider-surface.test.ts | 86 ++++++++++++++++++- 3 files changed, 110 insertions(+), 12 deletions(-) diff --git a/docs/architecture/06-integration-layer.md b/docs/architecture/06-integration-layer.md index a6477a57f..d4ef73a73 100644 --- a/docs/architecture/06-integration-layer.md +++ b/docs/architecture/06-integration-layer.md @@ -56,6 +56,8 @@ const integrationRegistry: IntegrationRegistry; // singleton The PM barrel (`src/integrations/pm/index.ts`) imports each provider once, then mirrors each manifest's `pmIntegration` into `integrationRegistry`. New PM providers add one provider folder plus one import in that barrel; shared router, worker, dashboard, CLI, and config files are guarded against provider-specific edits by conformance tests. +The dashboard has a matching frontend PM-provider boundary under `web/src/components/projects/pm-providers//`. A provider owns its wizard definition, auth metadata, credential persistence metadata, save config serialization, edit-mode hydration, discovery/mutation hooks, webhook UX composition, and provider-specific state slice in that folder. The shared wizard files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, and `pm-wizard-common-steps.tsx`) only render registered provider definitions and run metadata-driven verification/save helpers, so adding a provider does not require editing them. New providers add one frontend barrel import in `web/src/components/projects/pm-providers/index.ts`; `pm-wizard-state.ts` is the remaining explicit shared-dashboard exception while it composes provider state slices into the aggregate `WizardState` and reducer. + `src/pm/integration.ts` — extends `IntegrationModule` with PM-specific methods: - `createProvider(project)` — create a `PMProvider` instance for CRUD operations diff --git a/src/integrations/README.md b/src/integrations/README.md index 45e371a32..a828507a1 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -31,12 +31,14 @@ A new PM provider is ONE manifest backed by ONE provider folder + ONE wizard fol web/src/components/projects/pm-providers// index.ts // registerProviderWizard(ProviderWizard) on module load - wizard.ts // ProviderWizardDefinition (steps, save transform, completion predicates) - adapters.tsx // step-component adapters that bridge providerHooks → existing step props - steps.tsx // React components for each wizard step (or re-export from pm-wizard--steps.tsx) + wizard.ts // ProviderWizardDefinition (steps, save transform, edit hydration) + state.ts // provider-owned WizardState slice, actions, reducer, defaults + hooks.ts // provider-owned discovery/mutation/webhook hooks + auth wrappers + webhook-step.tsx // optional provider webhook UX composition around shared steps + steps.tsx // optional custom-step components only ``` -Nothing outside those two folders needs to change when you add a provider. The registries are the only surface the rest of the codebase sees. +The registries are the only surface the rest of the codebase sees. A new provider wires itself through the backend PM barrel and the frontend PM-provider barrel; it does not edit shared wizard orchestration (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, or `pm-wizard-common-steps.tsx`). The shared dashboard state file only composes provider-owned state slices and remains the temporary explicit exception documented in step 4 below. --- @@ -85,11 +87,20 @@ See [`web/src/components/projects/pm-providers/types.ts`](../../web/src/componen | `label` | Shown in the provider-select dropdown. | | `steps` | Array of `{ id, title, Component, isComplete }`. The generic wizard renders them in order. | | `buildIntegrationConfig` | Transforms wizard state into the integration config payload sent at save time. | +| `buildEditState` | Hydrates edit-mode state from saved config + configured credential keys. This is provider-owned and must never return raw credential values. | | `isSetupComplete` | `(state) => boolean`. True when the wizard can be saved. | | `useProviderHooks?` | Optional — composes the provider's React hooks (discovery, label creation, custom-field creation) inside a shell component. Return value flows into each step's `providerHooks` prop. | `ManifestProviderWizardSection` (`web/src/components/projects/pm-providers/manifest-section.tsx`) is the shell component that hosts the unconditional `useProviderHooks` call — it's only mounted when a manifest is registered for the active provider, so React's rules-of-hooks hold. +Frontend provider ownership is intentionally broad: + +- **State slice** — provider credential/config fields, action types, reducer cases, defaults, and provider-specific selectors live in `pm-providers//state.ts`. `pm-wizard-state.ts` composes those slices and is the only shared dashboard state surface that may need a new-provider edit while the shared state type still aggregates all provider fields. +- **Config serialization** — `ProviderWizardDefinition.buildIntegrationConfig` in `pm-providers//wizard.ts` is the only save-payload transform for that provider. +- **Edit hydration** — `ProviderWizardDefinition.buildEditState` restores saved provider config into wizard state and sets stored-credential flags from configured credential keys; shared `pm-wizard.tsx` only dispatches the returned partial state. +- **Auth metadata** — `auth`, `credentialPersistence`, and `formatVerificationDisplay` live in the provider wizard definition. Shared verification, credential readiness, mutation auth, and save code read this metadata instead of branching by provider. +- **Hooks and webhook UX** — discovery, label/custom-field creation, webhook list normalization, webhook create/delete controls, signing-secret fields, and provider-specific mutation wrappers live under `pm-providers//hooks.ts` and provider step adapters such as `webhook-step.tsx`. Shared `pm-wizard-hooks.ts` stays provider-agnostic. + --- ## Shared helpers (consume these; don't fork) @@ -204,13 +215,13 @@ All three real providers are now on the hardened contracts. Plan 009/4 also ship | Webhook-UX migration complete | Every PM wizard step, without exception, renders via the manifest path. Trello, JIRA, and Linear each own their webhook step via a per-provider adapter (`pm-providers//webhook-step.tsx`) — Fragment composition around the shared `WebhookUrlDisplayStep`. Trello + JIRA compose with programmatic "Create Webhook" button + active-webhooks list + delete + curl fallback (via existing `webhooks.create/list/delete({trelloOnly|jiraOnly:true})` tRPC endpoints). Linear composes with info banner + `ProjectSecretField` (`LINEAR_WEBHOOK_SECRET`) + 5-step manual setup instructions. | | Legacy deletions | `WebhookStep` + `LinearWebhookInfoPanel` + `useWebhookManagement` + `useLinearWebhookInfo` all deleted. `pm-wizard-common-steps.tsx` now only exports `SaveStep`. Legacy test file `pm-wizard-webhooks-step.test.ts` deleted — assertions moved into per-provider adapter tests. | | Parent-wizard filter | The `-webhook` id-skip filter (stopgap from plan 011/4) is gone. `renderedManifestSteps = manifestDef.steps.map(...)` — no filter. | -| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. New providers add one import to the frontend barrel (`web/src/components/projects/pm-providers/index.ts`) — the symmetric counterpart of the backend barrel — and `pm-wizard.tsx` picks it up automatically. The provider picker, edit hydration dispatch (`ProviderWizardDefinition.buildEditState`), verification-button readiness (`areCredentialsReadyFromMetadata`), and mutation auth path (`buildProviderAuthArgFromMetadata`) are all metadata-driven; no shared edits required beyond the barrel import. **Shared dashboard state** (`pm-wizard-state.ts`) must still be updated with the new provider's credential fields (`WizardState`) and reducer action types — see step 4 of "Adding a new PM provider" below. | +| New-provider guarantee | Adding a PM provider requires zero edits to `pm-wizard.tsx`, `pm-wizard-common-steps.tsx`, or `pm-wizard-hooks.ts`. New providers add one import to the frontend barrel (`web/src/components/projects/pm-providers/index.ts`) — the symmetric counterpart of the backend barrel — and `pm-wizard.tsx` picks it up automatically. The provider picker, edit hydration dispatch (`ProviderWizardDefinition.buildEditState`), config serialization (`ProviderWizardDefinition.buildIntegrationConfig`), verification-button readiness (`areCredentialsReadyFromMetadata`), mutation auth path (`buildProviderAuthArgFromMetadata`), and save credential persistence are all metadata/provider-definition driven; no shared edits required beyond the barrel import. **Shared dashboard state** (`pm-wizard-state.ts`) must still compose the new provider's state slice and action type — see step 4 of "Adding a new PM provider" below. | --- ## Adding a new PM provider (step by step) -Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. The orchestration and schema work lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The one shared dashboard file that still requires a new-provider edit is `pm-wizard-state.ts` (step 4 below) — new providers add credential fields to `WizardState` and action types to `WizardAction`. Edit-mode config hydration belongs on the provider's `ProviderWizardDefinition.buildEditState`. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces the shared-file invariant for orchestration and schema files; `pm-wizard-state.ts` is the deliberate exception. +Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / worker / CLI / dashboard / configMapper / central schema files**. The orchestration and schema work lives in your provider folder + your wizard folder + one import in `src/integrations/pm/index.ts` + one import in `web/src/components/projects/pm-providers/index.ts`. The shared wizard orchestration files (`pm-wizard.tsx`, `pm-wizard-hooks.ts`, `pm-wizard-common-steps.tsx`) are guarded shared surface and should not change for a new provider. The one shared dashboard file that still requires a new-provider edit is `pm-wizard-state.ts` (step 4 below) — it composes the provider's state slice into `WizardState`, `WizardAction`, initial state, and reducer delegation. Edit-mode config hydration belongs on the provider's `ProviderWizardDefinition.buildEditState`; save-payload serialization belongs on `ProviderWizardDefinition.buildIntegrationConfig`; provider hook auth and mutation wiring belong in provider-owned metadata/hooks. The `tests/unit/integrations/new-provider-surface.test.ts` guard enforces the shared-file invariant for orchestration and schema files; `pm-wizard-state.ts` is the deliberate exception. 1. **Backend folder** at `src/integrations/pm//`: - `client.ts` (or reuse a sibling under `src//`) — your REST / GraphQL client. Must use `withXxxCredentials()` + AsyncLocalStorage credential scoping; never hand-assemble Bearer headers (see `_shared/auth-headers.ts`). @@ -222,15 +233,16 @@ Spec 009 AC #10: **a new PM provider PR should not need to edit shared router / 2. **Wire the backend manifest** via a single import in `src/integrations/pm/index.ts` (`import './/index.js';`). No other backend file needs to change — the `single-entrypoint` test guards this. -3. **Frontend folder** at `web/src/components/projects/pm-providers//`: `wizard.ts` (`ProviderWizardDefinition` with `auth`, `credentialPersistence`, `formatVerificationDisplay`, and `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `hooks.ts` for provider-owned discovery/mutation wrappers, `auth.ts` for reusable auth metadata, and `index.ts` for side-effect registration (`registerProviderWizard(ProviderWizard)`). For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). Shared `pm-wizard-hooks.ts` remains limited to metadata-driven verification/save shells and provider-agnostic mutation factories. +3. **Frontend folder** at `web/src/components/projects/pm-providers//`: `wizard.ts` (`ProviderWizardDefinition` with `auth`, `credentialPersistence`, `formatVerificationDisplay`, `buildIntegrationConfig`, `buildEditState`, and `useProviderHooks` if the provider needs discovery / label creation / custom-field creation / webhook registration), `state.ts` for the provider-owned wizard state slice/actions/reducer/defaults, `hooks.ts` for provider-owned discovery/mutation/auth/webhook wrappers, `auth.ts` for reusable auth metadata when useful, and `index.ts` for side-effect registration (`registerProviderWizard(ProviderWizard)`). For shared wizard steps declared on `manifest.wizardSpec`, the generator in `pm-providers/generator.tsx` dispatches directly to the real shared step components at `pm-providers/steps/*.tsx` — there are **seven** kinds: `credentials`, `container-pick`, `status-mapping`, `label-mapping`, `webhook-url-display`, `project-scope`, `custom-field-mapping`. A provider with purely standard steps writes **zero** per-provider step components; Trello, JIRA, and Linear all use the shared components for every standard kind. Provide `providerHooks` (returned from `useProviderHooks`) to forward discovery data + mutation callbacks into the shared components; the generator spreads `ctx.providerHooks` as props. Unknown step `kind` values still warn-and-render a placeholder. **Provider-specific UI** ships either as (a) `kind: 'custom'` steps declared on the manifest and resolved to provider-folder components (Trello OAuth popup, JIRA issue-type mapping), or (b) Fragment compositions around a shared step when the base UX is standard but needs augmentation (Trello/JIRA webhook steps compose `WebhookUrlDisplayStep` + programmatic Create UX + active-webhook normalization; Linear composes `WebhookUrlDisplayStep` + `ProjectSecretField` + setup instructions — see `pm-providers/{trello,jira,linear}/webhook-step.tsx` for the reference composition pattern). Shared `pm-wizard-hooks.ts` remains limited to metadata-driven verification/save shells and provider-agnostic mutation factories. -4. **Update shared dashboard state** in `web/src/components/projects/pm-wizard-state.ts`. This is the one shared dashboard file a new provider must edit: - - Add the provider's raw credential fields to `WizardState` (e.g. `asanaApiKey: string`). - - Add the corresponding `SET__` action types to `WizardAction` (one per credential field); each reducer case should set `verificationResult: null, verifyError: null` alongside the updated field. +4. **Update shared dashboard state** in `web/src/components/projects/pm-wizard-state.ts`. This is the one shared dashboard file a new provider must edit while `WizardState` remains an aggregate type: + - Import the provider's state-slice helpers from `pm-providers//state.ts`. + - Compose the provider slice into `WizardState`, `WizardAction`, `createInitialState`, and `wizardReducer` delegation. + - Keep provider-specific credential fields, action definitions, defaults, and reducer logic inside the provider `state.ts`; reducer cases that mutate credentials should clear `verificationResult` and `verifyError`. Edit-mode hydration is provider-owned: implement `buildEditState(initialConfig, configuredKeys)` on the provider's `ProviderWizardDefinition`. It should set `hasStoredCredentials` from the relevant persisted credential keys and restore saved config values (container IDs, status/label mappings) without returning raw credential values. - The credential-readiness check (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are fully metadata-driven — they read `manifestDef.auth.rawCredentials` and require **no changes** here. + Save config serialization is also provider-owned: implement `buildIntegrationConfig(state)` on the provider's `ProviderWizardDefinition`. The credential-readiness check (`areCredentialsReadyFromMetadata` in `pm-wizard-hooks.ts`) and the mutation auth path (`buildProviderAuthArgFromMetadata`) are fully metadata-driven — they read `manifestDef.auth.rawCredentials` and require **no changes** here. 5. **Wire the frontend wizard** via a single import in `web/src/components/projects/pm-providers/index.ts` (`import './/index.js';`). This frontend barrel is the symmetric counterpart of the backend barrel at `src/integrations/pm/index.ts` — `pm-wizard.tsx` imports the barrel once and never needs to be edited for a new provider. diff --git a/tests/unit/integrations/new-provider-surface.test.ts b/tests/unit/integrations/new-provider-surface.test.ts index dff5f9edc..665c5f658 100644 --- a/tests/unit/integrations/new-provider-surface.test.ts +++ b/tests/unit/integrations/new-provider-surface.test.ts @@ -13,8 +13,19 @@ * guard, not a hard ban — if a contributor genuinely needs to extend * shared infrastructure (e.g., adding a new StandardStepKind), they * update the expected list below and explain why in the commit message. + * + * The three shared PM wizard orchestration files (pm-wizard.tsx, + * pm-wizard-hooks.ts, pm-wizard-common-steps.tsx) receive an additional + * SHA-256 content-hash guard (see GUARDED_WIZARD_FILE_HASHES below). + * Unlike the existence check, the hash check fails when the file is + * *modified*, not just when it is deleted — matching the documented + * invariant that adding a provider should never require editing them. + * Update a pinned hash only when the file genuinely needs to change for + * non-provider-specific reasons, and include the justification in the + * commit message. */ +import { createHash } from 'node:crypto'; import { readFileSync, statSync } from 'node:fs'; import { resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; @@ -44,6 +55,16 @@ const SHARED_SURFACE_FILES = [ 'src/api/routers/pm-discovery.ts', 'web/src/components/projects/pm-providers/generator.tsx', + // Shared PM wizard orchestration — provider picker, edit hydration + // dispatch, verification, save, and common save step are metadata-driven + // or provider-definition driven. New-provider frontend work belongs in + // web/src/components/projects/pm-providers//, with + // pm-wizard-state.ts as the only deliberate shared dashboard exception + // while it composes provider state slices into the aggregate WizardState. + 'web/src/components/projects/pm-wizard.tsx', + 'web/src/components/projects/pm-wizard-hooks.ts', + 'web/src/components/projects/pm-wizard-common-steps.tsx', + // Frontend PM provider barrel — new providers add one import here, // just like the backend barrel at src/integrations/pm/index.ts. // pm-wizard.tsx imports this barrel and never needs to change. @@ -69,6 +90,27 @@ const SHARED_SURFACE_FILES = [ 'src/db/repositories/configMapper.ts', ] as const; +/** + * Pinned SHA-256 content hashes for the shared PM wizard orchestration + * files that a new provider PR must NEVER modify. The existence check in + * SHARED_SURFACE_FILES above only catches deletion; these hashes catch any + * modification — matching the README/architecture claim that these files + * are "guarded shared surface". + * + * To update a hash (legitimate infrastructure change, not a new-provider + * addition): run `node -e "const c=require('crypto'),f=require('fs'); + * console.log(c.createHash('sha256').update(f.readFileSync('','utf8')).digest('hex'))"`, + * paste the new value here, and include the justification in the commit message. + */ +const GUARDED_WIZARD_FILE_HASHES: Record = { + 'web/src/components/projects/pm-wizard.tsx': + '402cc6829689f34dfec940034f8ba014fe14425671018d0455ad67145e6a0fb9', + 'web/src/components/projects/pm-wizard-hooks.ts': + '7aa07ab092695afdfd2ecc78345c097b569adce46e3eb928e8ee0bfa5d4131dd', + 'web/src/components/projects/pm-wizard-common-steps.tsx': + '0d9ca8bb56036687aed695b502be75ebf2a753195decb2e6b58f440c2abaa7c9', +}; + describe('new-provider-surface (plan 009/5 task 4, spec 009 AC #10)', () => { it.each(SHARED_SURFACE_FILES)('shared surface file exists: %s', (relativePath) => { const full = resolve(PROJECT_ROOT, relativePath); @@ -86,6 +128,43 @@ describe('new-provider-surface (plan 009/5 task 4, spec 009 AC #10)', () => { } }); + /** + * Content-hash guard for the shared PM wizard orchestration files. + * Unlike the existence check above, this assertion fails the moment + * any of the three guarded files is modified — making the "guarded + * shared surface" claim in the README and architecture docs accurate. + * + * A legitimate change to one of these files (e.g., fixing a shared + * wizard bug, adding a StandardStepKind) must update the corresponding + * hash in GUARDED_WIZARD_FILE_HASHES and include a justification in + * the commit message explaining why this is not a new-provider edit. + */ + it.each( + Object.entries(GUARDED_WIZARD_FILE_HASHES), + )('shared wizard orchestration file is unmodified: %s', (relativePath, expectedHash) => { + const full = resolve(PROJECT_ROOT, relativePath); + const content = readFileSync(full, 'utf8'); + const actualHash = createHash('sha256').update(content).digest('hex'); + expect( + actualHash, + [ + `Shared wizard orchestration file ${relativePath} has been modified.`, + `Expected SHA-256: ${expectedHash}`, + `Actual SHA-256: ${actualHash}`, + ``, + `Spec 009 AC #10: adding a new PM provider must NOT require editing`, + `pm-wizard.tsx, pm-wizard-hooks.ts, or pm-wizard-common-steps.tsx.`, + `All new-provider frontend work belongs in:`, + ` web/src/components/projects/pm-providers//`, + ` (wizard.ts, state.ts, hooks.ts, auth.ts, webhook-step.tsx, custom steps)`, + ``, + `If your change is a legitimate infrastructure edit (not a new-provider`, + `addition), update the hash in GUARDED_WIZARD_FILE_HASHES in this test`, + `and include a justification in the commit message.`, + ].join('\n'), + ).toBe(expectedHash); + }); + /** * The explanatory assertion — this is the one that surfaces when * the guard catches something. It always passes on a clean tree; @@ -99,9 +178,14 @@ describe('new-provider-surface (plan 009/5 task 4, spec 009 AC #10)', () => { 'required for a new provider lives in:', ' - src/integrations/pm//', ' - web/src/components/projects/pm-providers//', + ' (wizard.ts, state.ts, hooks.ts, auth.ts, webhook-step.tsx, custom steps)', ' - A single import line in src/integrations/pm/index.ts (backend barrel)', ' - A single import line in web/src/components/projects/pm-providers/index.ts (frontend barrel)', - 'If you need to edit one of the shared surface files above, ', + 'Shared pm-wizard.tsx, pm-wizard-hooks.ts, and pm-wizard-common-steps.tsx', + 'are intentionally provider-agnostic. The current explicit frontend', + 'exception is pm-wizard-state.ts, which composes provider-owned state', + 'slices from web/src/components/projects/pm-providers//state.ts.', + 'If you need to edit one of the guarded shared surface files above,', 'update this test with the justification and the new expected state.', ].join('\n'); expect(invariant.length).toBeGreaterThan(0); From ff941d148195c217e9d6356e3e7197adad7d0dfc Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 20:17:19 +0200 Subject: [PATCH 13/15] fix(pm): throw PM gadget runtime failures (#1326) Co-authored-by: Cascade Bot --- CHANGELOG.md | 2 + docs/architecture/07-gadgets.md | 2 + src/gadgets/README.md | 2 + src/gadgets/pm/core/deleteChecklistItem.ts | 2 +- src/gadgets/pm/core/listWorkItems.ts | 2 +- src/gadgets/pm/core/moveWorkItem.ts | 2 +- src/gadgets/pm/core/postComment.ts | 2 +- src/gadgets/pm/core/readWorkItem.ts | 2 +- src/gadgets/pm/core/updateChecklistItem.ts | 2 +- src/gadgets/pm/core/updateWorkItem.ts | 2 +- .../cascade-tools-stdout-cleanliness.test.ts | 3 ++ tests/unit/cli/pm/pm-commands.test.ts | 49 +++++++++++++++++++ .../pm/core/deleteChecklistItem.test.ts | 14 +++--- .../gadgets/pm/core/listWorkItems.test.ts | 6 +-- .../unit/gadgets/pm/core/moveWorkItem.test.ts | 41 ++++++++-------- .../unit/gadgets/pm/core/postComment.test.ts | 14 +++--- .../unit/gadgets/pm/core/readWorkItem.test.ts | 6 +-- .../pm/core/updateChecklistItem.test.ts | 14 +++--- .../gadgets/pm/core/updateWorkItem.test.ts | 8 +-- 19 files changed, 115 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88b87cf4e..8aae542e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **PM `cascade-tools` runtime failures now exit non-zero with structured failure envelopes.** PM core commands now throw on fatal provider/API failures, letting `createCLICommand()` emit `{"success":false,"error":{"type":"runtime","message":"..."}}` instead of wrapping prose like `Error posting comment: ...` inside `success:true` data. Intentional non-fatal PM outcomes such as guarded move no-ops and friction retry queueing remain successful command results. See Trello card [lU9mHLJT](https://trello.com/c/lU9mHLJT/686-friction-tooling-low-pm-post-comment-returned-success-envelope-with-embedded-400-error). + - **Native-tool prompt examples for enum, scalar, number, and primitive-array flags now render as runnable CLI syntax instead of JSON string literals.** Agent-facing guidance now shows forms such as `cascade-tools scm create-pr-review --event APPROVE` and repeatable primitive arrays as `--labels bug --labels docs`, while object and array-of-object flags continue to render shell-quoted JSON payloads. See Trello card [l9Sira7y](https://trello.com/c/l9Sira7y/685-friction-tooling-low-create-pr-review-docs-show-quoted-enum-but-cli-requires-raw-enum). - **`resolve-conflicts` agent no longer silently skips when GitHub's async mergeability computation hasn't resolved by the time the `pull_request` webhook is processed** (spec 020). `PRConflictDetectedTrigger` previously exhausted a 2×2s synchronous retry budget and silently discarded the event when `mergeable === null` — because GitHub never sends a follow-up webhook once mergeability resolves, the `resolve-conflicts` agent never fired. The trigger now returns `TriggerResult.deferredRecheck`, which causes the router to schedule a bare BullMQ delayed re-check job ~45s later via `scheduleCoalescedJob` (deduped per PR). The worker re-dispatches via the trigger registry to get fresh mergeability state. Multiple rapid webhooks for the same PR coalesce to a single re-check job. If mergeability is still `null` after the re-check fires, a Sentry event is captured under tag `mergeability_recheck_exhausted` and a WARN log is emitted — not a silent discard. Observed live on ucho/PR #329 (2026-05-07). See [spec 020](docs/specs/020-github-mergeability-deferred-recheck.md). diff --git a/docs/architecture/07-gadgets.md b/docs/architecture/07-gadgets.md index 21dedd7ad..0225854f3 100644 --- a/docs/architecture/07-gadgets.md +++ b/docs/architecture/07-gadgets.md @@ -136,6 +136,8 @@ The `cascade-tools` binary uses a separate oclif config (`bin/cascade-tools.js`) New domain commands should not add branches in these helpers. They declare behavior through their `ToolDefinition` metadata (`cliAliases`, examples, file input alternatives, auto-resolution), and the shared generators consume it. +Core functions passed to `createCLICommand()` own domain work only. On fatal runtime/API/provider failures they throw, and the shared factory converts that exception into the structured `{"success":false,"error":{"type":"runtime","message":"..."}}` stdout envelope plus exit code 1. A returned value is always serialized as successful `data`, so gadgets must not return sentinel error strings such as `Error reading work item: ...` for fatal failures. Non-fatal command states that are part of the contract, such as guarded PM move no-ops or friction retry queueing, remain successful returns. + ## Session State `src/gadgets/sessionState.ts` diff --git a/src/gadgets/README.md b/src/gadgets/README.md index 79d5b65d5..588814631 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -122,6 +122,8 @@ Envelope fields: You do not call `emitCliError` directly. The shared factory routes every failure through it automatically — your job is to make the declarative metadata (describe text, examples, aliases, file alternatives) rich enough that the auto-generated envelope is actually useful. +Core gadget functions must throw for fatal runtime/API/provider failures. Do not return sentinel prose such as `Error posting comment: ...`; `createCLICommand()` treats returned values as successful `data`, and thrown errors as runtime failure envelopes. Intentional non-fatal outcomes may still return structured data or explicit status text when they are part of the command contract, such as guarded PM move no-ops or friction reports queued for retry. + --- ## Shared CLI helper layout diff --git a/src/gadgets/pm/core/deleteChecklistItem.ts b/src/gadgets/pm/core/deleteChecklistItem.ts index cb57db423..a5c3e6fa0 100644 --- a/src/gadgets/pm/core/deleteChecklistItem.ts +++ b/src/gadgets/pm/core/deleteChecklistItem.ts @@ -9,6 +9,6 @@ export async function deleteChecklistItem( return `Checklist item ${checkItemId} deleted from work item ${workItemId}`; } catch (error) { const message = error instanceof Error ? error.message : String(error); - return `Error deleting checklist item: ${message}`; + throw new Error(`Error deleting checklist item: ${message}`); } } diff --git a/src/gadgets/pm/core/listWorkItems.ts b/src/gadgets/pm/core/listWorkItems.ts index 66e073fdd..12a60fd87 100644 --- a/src/gadgets/pm/core/listWorkItems.ts +++ b/src/gadgets/pm/core/listWorkItems.ts @@ -22,6 +22,6 @@ export async function listWorkItems(containerId: string): Promise { return result; } catch (error) { const message = error instanceof Error ? error.message : String(error); - return `Error listing work items: ${message}`; + throw new Error(`Error listing work items: ${message}`); } } diff --git a/src/gadgets/pm/core/moveWorkItem.ts b/src/gadgets/pm/core/moveWorkItem.ts index a805b930e..8466fae4c 100644 --- a/src/gadgets/pm/core/moveWorkItem.ts +++ b/src/gadgets/pm/core/moveWorkItem.ts @@ -45,6 +45,6 @@ export async function moveWorkItem(params: MoveWorkItemParams): Promise return `Work item ${params.workItemId} moved to ${params.destination} successfully`; } catch (error) { const message = error instanceof Error ? error.message : String(error); - return `Error moving work item: ${message}`; + throw new Error(`Error moving work item: ${message}`); } } diff --git a/src/gadgets/pm/core/postComment.ts b/src/gadgets/pm/core/postComment.ts index f4ec81b9c..836e532d4 100644 --- a/src/gadgets/pm/core/postComment.ts +++ b/src/gadgets/pm/core/postComment.ts @@ -33,6 +33,6 @@ export async function postComment(workItemId: string, text: string): Promise { ); expect(stdout).toMatch(ENVELOPE_START); + const envelope = JSON.parse(stdout.trim()); + expect(envelope.success).toBe(false); + expect(envelope.error.type).toBe('runtime'); expect(stdout).not.toContain(ESC); expect(stdout).not.toMatch(LOG_LEVEL_PREFIX); expect(stdout).not.toContain('[cascade]'); diff --git a/tests/unit/cli/pm/pm-commands.test.ts b/tests/unit/cli/pm/pm-commands.test.ts index 5e4f9a7cf..16ca9a03d 100644 --- a/tests/unit/cli/pm/pm-commands.test.ts +++ b/tests/unit/cli/pm/pm-commands.test.ts @@ -87,6 +87,14 @@ function makeMockConfig() { return { runHook: vi.fn().mockResolvedValue({ successes: [], failures: [] }) }; } +async function runExpectingExit(cmd: { run: () => Promise }): Promise { + try { + await cmd.run(); + } catch { + // emitCliError exits with code 1; oclif throws in the test environment. + } +} + afterEach(() => { vi.restoreAllMocks(); }); @@ -134,6 +142,25 @@ describe('ReadWorkItem command', () => { expect(output.success).toBe(true); expect(output.data).toEqual({ id: 'card-123', title: 'Test Card' }); }); + + it('outputs a runtime failure envelope when readWorkItem rejects', async () => { + vi.mocked(readWorkItem).mockRejectedValue(new Error('Request failed with status code 400')); + const cmd = new ReadWorkItem(['--workItemId', 'card-123'], makeMockConfig() as never); + const logSpy = vi.spyOn(cmd, 'log'); + const exitSpy = vi.spyOn(cmd, 'exit'); + + await runExpectingExit(cmd); + + expect(exitSpy).toHaveBeenCalledWith(1); + const output = JSON.parse(logSpy.mock.calls[0][0] as string); + expect(output).toEqual({ + success: false, + error: { + type: 'runtime', + message: 'Request failed with status code 400', + }, + }); + }); }); // --------------------------------------------------------------------------- @@ -399,4 +426,26 @@ describe('PostComment command (basic params)', () => { expect(output.success).toBe(true); expect(output.data).toEqual({ id: 'comment-new' }); }); + + it('outputs a runtime failure envelope when postComment rejects', async () => { + vi.mocked(postComment).mockRejectedValue(new Error('Request failed with status code 400')); + const cmd = new PostComment( + ['--workItemId', 'card-1', '--text', 'Hello world'], + makeMockConfig() as never, + ); + const logSpy = vi.spyOn(cmd, 'log'); + const exitSpy = vi.spyOn(cmd, 'exit'); + + await runExpectingExit(cmd); + + expect(exitSpy).toHaveBeenCalledWith(1); + const output = JSON.parse(logSpy.mock.calls[0][0] as string); + expect(output).toEqual({ + success: false, + error: { + type: 'runtime', + message: 'Request failed with status code 400', + }, + }); + }); }); diff --git a/tests/unit/gadgets/pm/core/deleteChecklistItem.test.ts b/tests/unit/gadgets/pm/core/deleteChecklistItem.test.ts index d31686162..5b93d8d4a 100644 --- a/tests/unit/gadgets/pm/core/deleteChecklistItem.test.ts +++ b/tests/unit/gadgets/pm/core/deleteChecklistItem.test.ts @@ -20,19 +20,19 @@ describe('deleteChecklistItem', () => { expect(result).toBe('Checklist item checkItem1 deleted from work item item1'); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.deleteChecklistItem.mockRejectedValue(new Error('API error')); - const result = await deleteChecklistItem('item1', 'checkItem1'); - - expect(result).toBe('Error deleting checklist item: API error'); + await expect(deleteChecklistItem('item1', 'checkItem1')).rejects.toThrow( + 'Error deleting checklist item: API error', + ); }); it('handles non-Error thrown value', async () => { mockProvider.deleteChecklistItem.mockRejectedValue('string error'); - const result = await deleteChecklistItem('item1', 'ci1'); - - expect(result).toBe('Error deleting checklist item: string error'); + await expect(deleteChecklistItem('item1', 'ci1')).rejects.toThrow( + 'Error deleting checklist item: string error', + ); }); }); diff --git a/tests/unit/gadgets/pm/core/listWorkItems.test.ts b/tests/unit/gadgets/pm/core/listWorkItems.test.ts index 144e9747e..b11f77f65 100644 --- a/tests/unit/gadgets/pm/core/listWorkItems.test.ts +++ b/tests/unit/gadgets/pm/core/listWorkItems.test.ts @@ -100,11 +100,9 @@ describe('listWorkItems', () => { expect(result).not.toContain('...'); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.listWorkItems.mockRejectedValue(new Error('API error')); - const result = await listWorkItems('list1'); - - expect(result).toBe('Error listing work items: API error'); + await expect(listWorkItems('list1')).rejects.toThrow('Error listing work items: API error'); }); }); diff --git a/tests/unit/gadgets/pm/core/moveWorkItem.test.ts b/tests/unit/gadgets/pm/core/moveWorkItem.test.ts index 395bb3a76..480c95b01 100644 --- a/tests/unit/gadgets/pm/core/moveWorkItem.test.ts +++ b/tests/unit/gadgets/pm/core/moveWorkItem.test.ts @@ -23,26 +23,26 @@ describe('moveWorkItem', () => { expect(result).toBe('Work item card1 moved to list2 successfully'); }); - it('returns error string on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.moveWorkItem.mockRejectedValue(new Error('API error')); - const result = await moveWorkItem({ - workItemId: 'card1', - destination: 'list2', - }); - - expect(result).toBe('Error moving work item: API error'); + await expect( + moveWorkItem({ + workItemId: 'card1', + destination: 'list2', + }), + ).rejects.toThrow('Error moving work item: API error'); }); it('handles non-Error throws', async () => { mockProvider.moveWorkItem.mockRejectedValue('network timeout'); - const result = await moveWorkItem({ - workItemId: 'card1', - destination: 'list2', - }); - - expect(result).toBe('Error moving work item: network timeout'); + await expect( + moveWorkItem({ + workItemId: 'card1', + destination: 'list2', + }), + ).rejects.toThrow('Error moving work item: network timeout'); }); // ── expectedSourceState guard ──────────────────────────────────────────── @@ -144,17 +144,18 @@ describe('moveWorkItem', () => { expect(mockProvider.moveWorkItem).toHaveBeenCalledWith('card1', 'list2'); }); - it('returns a structured error if getWorkItem throws', async () => { + it('throws a structured error if getWorkItem throws', async () => { mockProvider.getWorkItem.mockRejectedValue(new Error('API down')); - const result = await moveWorkItem({ - workItemId: 'MNG-538', - destination: 'todo-state-id', - expectedSourceState: 'Backlog', - }); + await expect( + moveWorkItem({ + workItemId: 'MNG-538', + destination: 'todo-state-id', + expectedSourceState: 'Backlog', + }), + ).rejects.toThrow('Error moving work item: API down'); expect(mockProvider.moveWorkItem).not.toHaveBeenCalled(); - expect(result).toContain('Error'); }); }); }); diff --git a/tests/unit/gadgets/pm/core/postComment.test.ts b/tests/unit/gadgets/pm/core/postComment.test.ts index 64f7eb00d..8618d13a2 100644 --- a/tests/unit/gadgets/pm/core/postComment.test.ts +++ b/tests/unit/gadgets/pm/core/postComment.test.ts @@ -48,12 +48,12 @@ describe('postComment', () => { expect(result).toBe('Comment posted successfully'); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.addComment.mockRejectedValue(new Error('Network error')); - const result = await postComment('item1', 'text'); - - expect(result).toBe('Error posting comment: Network error'); + await expect(postComment('item1', 'text')).rejects.toThrow( + 'Error posting comment: Network error', + ); }); it('passes multi-line text correctly', async () => { @@ -68,9 +68,9 @@ describe('postComment', () => { it('handles non-Error thrown value', async () => { mockProvider.addComment.mockRejectedValue('string error'); - const result = await postComment('item1', 'text'); - - expect(result).toBe('Error posting comment: string error'); + await expect(postComment('item1', 'text')).rejects.toThrow( + 'Error posting comment: string error', + ); }); describe('progress comment replacement', () => { diff --git a/tests/unit/gadgets/pm/core/readWorkItem.test.ts b/tests/unit/gadgets/pm/core/readWorkItem.test.ts index 4767c99fd..d04e3c0fb 100644 --- a/tests/unit/gadgets/pm/core/readWorkItem.test.ts +++ b/tests/unit/gadgets/pm/core/readWorkItem.test.ts @@ -159,12 +159,10 @@ describe('readWorkItem', () => { expect(mockProvider.getWorkItemComments).not.toHaveBeenCalled(); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.getWorkItem.mockRejectedValue(new Error('Network error')); - const result = await readWorkItem('item1'); - - expect(result).toContain('Error reading work item: Network error'); + await expect(readWorkItem('item1')).rejects.toThrow('Error reading work item: Network error'); }); it('handles label without color', async () => { diff --git a/tests/unit/gadgets/pm/core/updateChecklistItem.test.ts b/tests/unit/gadgets/pm/core/updateChecklistItem.test.ts index 99619a59e..da22e30ef 100644 --- a/tests/unit/gadgets/pm/core/updateChecklistItem.test.ts +++ b/tests/unit/gadgets/pm/core/updateChecklistItem.test.ts @@ -29,19 +29,19 @@ describe('updateChecklistItem', () => { expect(result).toBe('Checklist item checkItem1 marked incomplete on work item item1'); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.updateChecklistItem.mockRejectedValue(new Error('API error')); - const result = await updateChecklistItem('item1', 'checkItem1', true); - - expect(result).toBe('Error updating checklist item: API error'); + await expect(updateChecklistItem('item1', 'checkItem1', true)).rejects.toThrow( + 'Error updating checklist item: API error', + ); }); it('handles non-Error thrown value', async () => { mockProvider.updateChecklistItem.mockRejectedValue('string error'); - const result = await updateChecklistItem('item1', 'ci1', false); - - expect(result).toBe('Error updating checklist item: string error'); + await expect(updateChecklistItem('item1', 'ci1', false)).rejects.toThrow( + 'Error updating checklist item: string error', + ); }); }); diff --git a/tests/unit/gadgets/pm/core/updateWorkItem.test.ts b/tests/unit/gadgets/pm/core/updateWorkItem.test.ts index 6a28eb65e..12f889c4d 100644 --- a/tests/unit/gadgets/pm/core/updateWorkItem.test.ts +++ b/tests/unit/gadgets/pm/core/updateWorkItem.test.ts @@ -75,12 +75,12 @@ describe('updateWorkItem', () => { expect(result).toBe('Work item updated: title, description, 1 label(s)'); }); - it('returns error message on failure', async () => { + it('throws an error message on failure', async () => { mockProvider.updateWorkItem.mockRejectedValue(new Error('API error')); - const result = await updateWorkItem({ workItemId: 'item1', title: 'T' }); - - expect(result).toBe('Error updating work item: API error'); + await expect(updateWorkItem({ workItemId: 'item1', title: 'T' })).rejects.toThrow( + 'Error updating work item: API error', + ); }); it('does not call updateWorkItem when only labels provided', async () => { From 1a69ef86cde0f9edf2e45044588a0e37de18436e Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 20:34:25 +0200 Subject: [PATCH 14/15] fix(scm): accept body-file for PR reviews (#1327) Co-authored-by: Cascade Bot --- CHANGELOG.md | 2 + src/gadgets/README.md | 2 +- src/gadgets/github/definitions.ts | 5 ++ .../backends/shared-nativeToolPrompts.test.ts | 7 ++ tests/unit/cli/file-input-flags.test.ts | 83 +++++++++++++++++++ tests/unit/gadgets/github/definitions.test.ts | 9 +- 6 files changed, 106 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8aae542e6..26221e7e5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **`cascade-tools scm create-pr-review` now accepts `--body-file ` and `--body-file -`.** This matches the generated CreatePRReview guidance and the existing CreatePR / PostPRComment file-input pattern for long Markdown bodies. See Trello card [7kmo42o6](https://trello.com/c/7kmo42o6/691-friction-tooling-low-createprreview-docs-advertise-body-file-but-cli-rejects-it). + - **PM `cascade-tools` runtime failures now exit non-zero with structured failure envelopes.** PM core commands now throw on fatal provider/API failures, letting `createCLICommand()` emit `{"success":false,"error":{"type":"runtime","message":"..."}}` instead of wrapping prose like `Error posting comment: ...` inside `success:true` data. Intentional non-fatal PM outcomes such as guarded move no-ops and friction retry queueing remain successful command results. See Trello card [lU9mHLJT](https://trello.com/c/lU9mHLJT/686-friction-tooling-low-pm-post-comment-returned-success-envelope-with-embedded-400-error). - **Native-tool prompt examples for enum, scalar, number, and primitive-array flags now render as runnable CLI syntax instead of JSON string literals.** Agent-facing guidance now shows forms such as `cascade-tools scm create-pr-review --event APPROVE` and repeatable primitive arrays as `--labels bug --labels docs`, while object and array-of-object flags continue to render shell-quoted JSON payloads. See Trello card [l9Sira7y](https://trello.com/c/l9Sira7y/685-friction-tooling-low-create-pr-review-docs-show-quoted-enum-but-cli-requires-raw-enum). diff --git a/src/gadgets/README.md b/src/gadgets/README.md index 588814631..9e0c3f250 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -168,7 +168,7 @@ Two tuning constants live in `src/gadgets/shared/cli/parseErrors.ts`: `MAX_FLAG_ ## Reference: `createPRReviewDef` -`src/gadgets/github/definitions.ts` is the reference gadget for spec 014 — it carries `cliAliases: ['comment']` on `comments`, a `fileInputAlternatives` entry for `--comments-file`, and a well-formed `examples` block. Read it before you add a new gadget with array-of-object parameters. +`src/gadgets/github/definitions.ts` is the reference gadget for spec 014 — it carries `cliAliases: ['comment']` on `comments`, `fileInputAlternatives` entries for `--body-file` (long review summaries) and `--comments-file` (JSON inline comments), and a well-formed `examples` block. Read it before you add a new gadget with array-of-object parameters. ## Reference: `ReportFriction` diff --git a/src/gadgets/github/definitions.ts b/src/gadgets/github/definitions.ts index 53fba219a..dc81f2eeb 100644 --- a/src/gadgets/github/definitions.ts +++ b/src/gadgets/github/definitions.ts @@ -237,6 +237,11 @@ export const createPRReviewDef: ToolDefinition = { cli: { autoResolved: ownerRepoAutoResolved, fileInputAlternatives: [ + { + paramName: 'body', + fileFlag: 'body-file', + description: 'Read review body from file (use - for stdin)', + }, { paramName: 'comments', fileFlag: 'comments-file', diff --git a/tests/unit/backends/shared-nativeToolPrompts.test.ts b/tests/unit/backends/shared-nativeToolPrompts.test.ts index 2b73a439f..2e6b422f4 100644 --- a/tests/unit/backends/shared-nativeToolPrompts.test.ts +++ b/tests/unit/backends/shared-nativeToolPrompts.test.ts @@ -171,6 +171,13 @@ describe('buildToolGuidance', () => { expect(result).toContain('# example: --event APPROVE'); expect(result).not.toContain(`--event '"APPROVE"'`); }); + + it('renders CreatePRReview body-file guidance from definition metadata', () => { + const result = buildToolGuidance([generateToolManifest(createPRReviewDef)]); + + expect(result).toContain('[--body-file ]'); + expect(result).toContain('Read review body from file (use - for stdin)'); + }); }); describe('formatParam — optional string param', () => { diff --git a/tests/unit/cli/file-input-flags.test.ts b/tests/unit/cli/file-input-flags.test.ts index 327a78558..2df3b30d8 100644 --- a/tests/unit/cli/file-input-flags.test.ts +++ b/tests/unit/cli/file-input-flags.test.ts @@ -39,6 +39,11 @@ vi.mock('../../../src/gadgets/pm/core/postComment.js', () => ({ vi.mock('../../../src/gadgets/github/core/createPR.js', () => ({ createPR: vi.fn().mockResolvedValue({ url: 'https://github.com/o/r/pull/1' }), })); +vi.mock('../../../src/gadgets/github/core/createPRReview.js', () => ({ + createPRReview: vi + .fn() + .mockResolvedValue({ reviewUrl: 'https://github.com/o/r/pull/1#review', event: 'COMMENT' }), +})); vi.mock('../../../src/gadgets/github/core/postPRComment.js', () => ({ postPRComment: vi.fn().mockResolvedValue({ id: 123 }), })); @@ -48,8 +53,10 @@ import PostComment from '../../../src/cli/pm/post-comment.js'; import ReportFriction from '../../../src/cli/pm/report-friction.js'; import UpdateWorkItem from '../../../src/cli/pm/update-work-item.js'; import CreatePR from '../../../src/cli/scm/create-pr.js'; +import CreatePRReview from '../../../src/cli/scm/create-pr-review.js'; import PostPRComment from '../../../src/cli/scm/post-pr-comment.js'; import { createPR } from '../../../src/gadgets/github/core/createPR.js'; +import { createPRReview } from '../../../src/gadgets/github/core/createPRReview.js'; import { postPRComment } from '../../../src/gadgets/github/core/postPRComment.js'; import { createWorkItem } from '../../../src/gadgets/pm/core/createWorkItem.js'; import { postComment } from '../../../src/gadgets/pm/core/postComment.js'; @@ -352,6 +359,82 @@ describe('CreatePR --body-file', () => { }); }); +describe('CreatePRReview --body-file', () => { + const originalEnv = process.env; + + beforeEach(() => { + process.env = { + ...originalEnv, + CASCADE_REPO_OWNER: 'owner', + CASCADE_REPO_NAME: 'repo', + }; + }); + + afterEach(() => { + process.env = originalEnv; + }); + + it('reads review body from file', async () => { + const filePath = writeTempFile('review.md', 'Review body from file'); + const cmd = new CreatePRReview( + ['--prNumber', '42', '--event', 'COMMENT', '--body-file', filePath], + mockConfig as never, + ); + await cmd.run(); + + expect(createPRReview).toHaveBeenCalledWith( + expect.objectContaining({ + owner: 'owner', + repo: 'repo', + prNumber: 42, + event: 'COMMENT', + body: 'Review body from file', + }), + ); + }); + + it('prefers --body-file over --body', async () => { + const filePath = writeTempFile('review.md', 'from file'); + const cmd = new CreatePRReview( + ['--prNumber', '42', '--event', 'APPROVE', '--body', 'from flag', '--body-file', filePath], + mockConfig as never, + ); + await cmd.run(); + + expect(createPRReview).toHaveBeenCalledWith( + expect.objectContaining({ + body: 'from file', + }), + ); + }); + + it('keeps inline comments JSON parsing when combined with --body-file', async () => { + const filePath = writeTempFile('review.md', 'Needs a small change'); + const comments = [{ path: 'src/index.ts', line: 12, body: 'Please handle null here.' }]; + const cmd = new CreatePRReview( + [ + '--prNumber', + '42', + '--event', + 'REQUEST_CHANGES', + '--body-file', + filePath, + '--comments', + JSON.stringify(comments), + ], + mockConfig as never, + ); + await cmd.run(); + + expect(createPRReview).toHaveBeenCalledWith( + expect.objectContaining({ + body: 'Needs a small change', + comments, + }), + ); + }); +}); + describe('PostPRComment --body-file', () => { const originalEnv = process.env; diff --git a/tests/unit/gadgets/github/definitions.test.ts b/tests/unit/gadgets/github/definitions.test.ts index e9ba5f002..2d1cce598 100644 --- a/tests/unit/gadgets/github/definitions.test.ts +++ b/tests/unit/gadgets/github/definitions.test.ts @@ -281,9 +281,16 @@ describe('createPRReviewDef — spec 014 opt-in', () => { expect(comments.cliAliases).toEqual(['comment']); }); - it('cli.fileInputAlternatives includes a comments-file entry with parseAs:"json"', () => { + it('cli.fileInputAlternatives includes body-file and comments-file entries', () => { const alts = createPRReviewDef.cli?.fileInputAlternatives ?? []; + const bodyFile = alts.find((a) => a.paramName === 'body'); const commentsFile = alts.find((a) => a.paramName === 'comments'); + + expect(bodyFile).toBeDefined(); + expect(bodyFile?.fileFlag).toBe('body-file'); + expect(bodyFile?.parseAs).toBeUndefined(); + expect(bodyFile?.description).toBeTruthy(); + expect(commentsFile).toBeDefined(); expect(commentsFile?.fileFlag).toBe('comments-file'); expect(commentsFile?.parseAs).toBe('json'); From e8aec91a2398b48863484e1bbe66b2fec32e2186 Mon Sep 17 00:00:00 2001 From: aaight Date: Sun, 10 May 2026 20:54:44 +0200 Subject: [PATCH 15/15] fix(cli): render ReadWorkItem IDs without literal quotes (#1328) Co-authored-by: Cascade Bot --- CHANGELOG.md | 2 ++ src/backends/shared/nativeToolPrompts.ts | 25 +------------------ src/cli/pm/read-work-item.ts | 16 +++++++++++- src/gadgets/README.md | 5 ++-- src/gadgets/shared/cli/examples.ts | 12 ++++----- src/gadgets/shared/cli/shellValues.ts | 23 +++++++++++++++++ .../backends/shared-nativeToolPrompts.test.ts | 10 ++++++++ tests/unit/cli/cascade-tools-help.test.ts | 3 +++ tests/unit/cli/pm/pm-commands.test.ts | 17 +++++++++++++ .../unit/gadgets/shared/cli/examples.test.ts | 13 ++++++++-- .../shared/promptExamplesGrammar.test.ts | 11 ++++++++ 11 files changed, 102 insertions(+), 35 deletions(-) create mode 100644 src/gadgets/shared/cli/shellValues.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 26221e7e5..3ef512989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ All notable user-visible changes to CASCADE are documented here. The format is l ### Fixed +- **`ReadWorkItem` examples now render PM IDs as runnable bare CLI values.** Native-tool prompt guidance and `cascade-tools pm read-work-item --help` now show `--workItemId abc123` instead of JSON-string-literal forms like `--workItemId '"abc123"'`. The CLI also strips one accidental outer quote layer for `ReadWorkItem` IDs only, so a copied bad example no longer sends literal quote characters to the PM provider. See Trello card [M5f9T1D7](https://trello.com/c/M5f9T1D7/673-frictionlow-readworkitem-example-quoting-produced-trello-card-id-with-literal-quotes). + - **`cascade-tools scm create-pr-review` now accepts `--body-file ` and `--body-file -`.** This matches the generated CreatePRReview guidance and the existing CreatePR / PostPRComment file-input pattern for long Markdown bodies. See Trello card [7kmo42o6](https://trello.com/c/7kmo42o6/691-friction-tooling-low-createprreview-docs-advertise-body-file-but-cli-rejects-it). - **PM `cascade-tools` runtime failures now exit non-zero with structured failure envelopes.** PM core commands now throw on fatal provider/API failures, letting `createCLICommand()` emit `{"success":false,"error":{"type":"runtime","message":"..."}}` instead of wrapping prose like `Error posting comment: ...` inside `success:true` data. Intentional non-fatal PM outcomes such as guarded move no-ops and friction retry queueing remain successful command results. See Trello card [lU9mHLJT](https://trello.com/c/lU9mHLJT/686-friction-tooling-low-pm-post-comment-returned-success-envelope-with-embedded-400-error). diff --git a/src/backends/shared/nativeToolPrompts.ts b/src/backends/shared/nativeToolPrompts.ts index 40c397e44..f56d75ab3 100644 --- a/src/backends/shared/nativeToolPrompts.ts +++ b/src/backends/shared/nativeToolPrompts.ts @@ -1,3 +1,4 @@ +import { formatJsonExample, formatShellScalar } from '../../gadgets/shared/cli/shellValues.js'; import type { ContextInjection, ToolManifest } from '../types.js'; import { buildInlineContextSection, offloadLargeContext } from './contextFiles.js'; @@ -25,30 +26,6 @@ type PromptParamSchema = { example?: unknown; }; -const SHELL_SAFE_VALUE_PATTERN = /^[A-Za-z0-9_./:@%+=,-]+$/; - -function shellQuote(value: string): string { - return `'${value.replace(/'/g, `'"'"'`)}'`; -} - -function formatShellScalar(value: unknown): string { - const rendered = String(value); - if (rendered.length > 0 && SHELL_SAFE_VALUE_PATTERN.test(rendered)) { - return rendered; - } - return shellQuote(rendered); -} - -function formatJsonExample(value: unknown): string | undefined { - try { - return shellQuote(JSON.stringify(value)); - } catch { - // JSON.stringify throws on cyclic refs — never in our tool definitions, - // but be defensive so a malformed example never crashes prompt building. - return undefined; - } -} - function formatExampleInvocation(key: string, schema: PromptParamSchema): string | undefined { if (schema.example === undefined) return undefined; diff --git a/src/cli/pm/read-work-item.ts b/src/cli/pm/read-work-item.ts index d349b10fe..9f4886243 100644 --- a/src/cli/pm/read-work-item.ts +++ b/src/cli/pm/read-work-item.ts @@ -2,6 +2,20 @@ import { readWorkItem } from '../../gadgets/pm/core/readWorkItem.js'; import { readWorkItemDef } from '../../gadgets/pm/definitions.js'; import { createCLICommand } from '../../gadgets/shared/cliCommandFactory.js'; +function normalizeWorkItemId(workItemId: string): string { + if (workItemId.length >= 2) { + const first = workItemId[0]; + const last = workItemId.at(-1); + if ((first === '"' || first === "'") && first === last) { + return workItemId.slice(1, -1); + } + } + return workItemId; +} + export default createCLICommand(readWorkItemDef, async (params) => { - return readWorkItem(params.workItemId as string, params.includeComments as boolean | undefined); + return readWorkItem( + normalizeWorkItemId(params.workItemId as string), + params.includeComments as boolean | undefined, + ); }); diff --git a/src/gadgets/README.md b/src/gadgets/README.md index 9e0c3f250..b0848c2d0 100644 --- a/src/gadgets/README.md +++ b/src/gadgets/README.md @@ -67,7 +67,7 @@ A list of `{ params, comment, output? }` invocations. The first example that pop - The `cascade-tools … --help` output lists every example as a runnable shell invocation under an `EXAMPLES` section. - JSON-parse failures include the example as the `expected` shape fragment in the structured error envelope. -Write examples that a model could literally copy/paste. Use double-quoted JSON keys; do not rely on the agent to translate pseudo-JSON. +Write examples that a model could literally copy/paste. Use double-quoted JSON keys; do not rely on the agent to translate pseudo-JSON. Shell-safe scalar IDs and names should stay bare (`--workItemId abc123`, `--owner acme`, `--prNumber 42`), not wrapped in quote characters. The shared renderer adds shell quotes only when a scalar contains spaces, shell metacharacters, or embedded quotes. Enum examples must be raw CLI values, not JSON strings. For example, the review action should render as: @@ -133,7 +133,8 @@ Core gadget functions must throw for fatal runtime/API/provider failures. Do not | Helper | Responsibility | |---|---| | `commandNames.ts` | Kebab-case conversion and `cascade-tools ` derivation shared with manifest generation | -| `examples.ts` | Example lookup, shell quoting, oclif example rendering, and JSON expected-shape hints | +| `examples.ts` | Example lookup, oclif example rendering, and JSON expected-shape hints | +| `shellValues.ts` | Shared shell-safe scalar and JSON payload formatting for CLI help and native-tool prompts | | `flags.ts` | oclif flag construction plus candidate and boolean-flag metadata collection | | `booleanArgv.ts` | Natural boolean value forms such as `--flag true`, `--flag=false`, `yes/no`, and `1/0` | | `parseErrors.ts` | oclif parse-error classification and unknown-flag suggestions | diff --git a/src/gadgets/shared/cli/examples.ts b/src/gadgets/shared/cli/examples.ts index cb80b2ebe..8bfcc8c74 100644 --- a/src/gadgets/shared/cli/examples.ts +++ b/src/gadgets/shared/cli/examples.ts @@ -1,4 +1,5 @@ import type { ParameterDefinition, ToolDefinition, ToolExample } from '../toolDefinition.js'; +import { formatJsonExample, formatShellScalar, shellQuote } from './shellValues.js'; /** * Pull the first concrete value for `paramName` out of the tool definition's examples block. @@ -39,21 +40,20 @@ function formatExampleLine(cliCommand: string, def: ToolDefinition, ex: ToolExam continue; } if (paramDef.type === 'array' && paramDef.items === 'string' && Array.isArray(value)) { - for (const v of value) parts.push(`--${key} ${shellQuote(String(v))}`); + for (const v of value) parts.push(`--${key} ${formatShellScalar(v)}`); continue; } if (paramDef.type === 'object' || (paramDef.type === 'array' && paramDef.items === 'object')) { - parts.push(`--${key} ${shellQuote(JSON.stringify(value))}`); + const json = formatJsonExample(value); + if (json) parts.push(`--${key} ${json}`); continue; } - parts.push(`--${key} ${shellQuote(String(value))}`); + parts.push(`--${key} ${formatShellScalar(value)}`); } return parts.join(' '); } -export function shellQuote(s: string): string { - return `'${s.replace(/'/g, `'\\''`)}'`; -} +export { shellQuote }; /** * Derive an `expected` shape hint for a JSON-parse failure envelope from the diff --git a/src/gadgets/shared/cli/shellValues.ts b/src/gadgets/shared/cli/shellValues.ts new file mode 100644 index 000000000..123a68799 --- /dev/null +++ b/src/gadgets/shared/cli/shellValues.ts @@ -0,0 +1,23 @@ +const SHELL_SAFE_VALUE_PATTERN = /^[A-Za-z0-9_./:@%+=,-]+$/; + +export function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + +export function formatShellScalar(value: unknown): string { + const rendered = String(value); + if (rendered.length > 0 && SHELL_SAFE_VALUE_PATTERN.test(rendered)) { + return rendered; + } + return shellQuote(rendered); +} + +export function formatJsonExample(value: unknown): string | undefined { + try { + return shellQuote(JSON.stringify(value)); + } catch { + // JSON.stringify throws on cyclic refs. Tool definition examples should + // never be cyclic, but prompt/help rendering should not crash if one is. + return undefined; + } +} diff --git a/tests/unit/backends/shared-nativeToolPrompts.test.ts b/tests/unit/backends/shared-nativeToolPrompts.test.ts index 2e6b422f4..e788c7ad5 100644 --- a/tests/unit/backends/shared-nativeToolPrompts.test.ts +++ b/tests/unit/backends/shared-nativeToolPrompts.test.ts @@ -26,6 +26,7 @@ import { buildToolGuidance, } from '../../../src/backends/shared/nativeToolPrompts.js'; import { createPRReviewDef } from '../../../src/gadgets/github/definitions.js'; +import { readWorkItemDef } from '../../../src/gadgets/pm/definitions.js'; import { generateToolManifest } from '../../../src/gadgets/shared/manifestGenerator.js'; // ───────── helper ───────── @@ -172,6 +173,15 @@ describe('buildToolGuidance', () => { expect(result).not.toContain(`--event '"APPROVE"'`); }); + it('renders ReadWorkItem work item IDs as bare shell-safe values', () => { + const result = buildToolGuidance([generateToolManifest(readWorkItemDef)]); + + expect(result).toContain('# example: --workItemId abc123'); + expect(result).not.toContain(`--workItemId '"abc123"'`); + expect(result).not.toContain(`--workItemId 'abc123'`); + expect(result).not.toContain(`--workItemId "abc123"`); + }); + it('renders CreatePRReview body-file guidance from definition metadata', () => { const result = buildToolGuidance([generateToolManifest(createPRReviewDef)]); diff --git a/tests/unit/cli/cascade-tools-help.test.ts b/tests/unit/cli/cascade-tools-help.test.ts index 397fd0efc..0e460b2d5 100644 --- a/tests/unit/cli/cascade-tools-help.test.ts +++ b/tests/unit/cli/cascade-tools-help.test.ts @@ -72,5 +72,8 @@ describe('cascade-tools --help — topic summaries', () => { expect(stdout).toContain('Read a work item'); expect(stdout).toContain('--workItemId'); expect(stdout).toContain('--[no-]includeComments'); + expect(stdout).toContain('cascade-tools pm read-work-item --workItemId abc123'); + expect(stdout).not.toContain(`--workItemId '"abc123"'`); + expect(stdout).not.toContain(`--workItemId 'abc123'`); }); }); diff --git a/tests/unit/cli/pm/pm-commands.test.ts b/tests/unit/cli/pm/pm-commands.test.ts index 16ca9a03d..d9af24b00 100644 --- a/tests/unit/cli/pm/pm-commands.test.ts +++ b/tests/unit/cli/pm/pm-commands.test.ts @@ -111,6 +111,23 @@ describe('ReadWorkItem command', () => { expect(readWorkItem).toHaveBeenCalledWith('card-123', true); }); + it('normalizes one accidental outer quote layer around workItemId', async () => { + const doubleQuoted = new ReadWorkItem( + ['--workItemId', '"card-123"'], + makeMockConfig() as never, + ); + await doubleQuoted.run(); + + const singleQuoted = new ReadWorkItem( + ['--workItemId', "'card-456'"], + makeMockConfig() as never, + ); + await singleQuoted.run(); + + expect(readWorkItem).toHaveBeenNthCalledWith(1, 'card-123', true); + expect(readWorkItem).toHaveBeenNthCalledWith(2, 'card-456', true); + }); + it('passes includeComments=true when --includeComments is set', async () => { const cmd = new ReadWorkItem( ['--workItemId', 'card-123', '--includeComments'], diff --git a/tests/unit/gadgets/shared/cli/examples.test.ts b/tests/unit/gadgets/shared/cli/examples.test.ts index ad2442b51..fd9694d65 100644 --- a/tests/unit/gadgets/shared/cli/examples.test.ts +++ b/tests/unit/gadgets/shared/cli/examples.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; - +import { readWorkItemDef } from '../../../../../src/gadgets/pm/definitions.js'; import { buildOclifExamples, expectedShapeFor, @@ -47,11 +47,20 @@ describe('CLI examples', () => { const [line] = buildOclifExamples(reviewDef, 'cascade-tools scm create-pr-review'); expect(line).toContain("cascade-tools scm create-pr-review --body 'Don'\\''t merge yet'"); expect(line).toContain('--no-draft'); - expect(line).toContain("--labels 'needs-work' --labels 'api'"); + expect(line).toContain('--labels needs-work --labels api'); expect(line).toContain('--comments \'[{"path":"src/a.ts","line":3,"body":"nit"}]\''); expect(line).not.toContain('hidden'); }); + it('renders ReadWorkItem examples with shell-safe IDs as bare values', () => { + const [line] = buildOclifExamples(readWorkItemDef, 'cascade-tools pm read-work-item'); + + expect(line).toBe('cascade-tools pm read-work-item --workItemId abc123 --includeComments'); + expect(line).not.toContain(`--workItemId '"abc123"'`); + expect(line).not.toContain(`--workItemId 'abc123'`); + expect(line).not.toContain(`--workItemId "abc123"`); + }); + it('uses examples as JSON expected-shape hints with describe fallback', () => { const paramDef = reviewDef.parameters.comments; expect(expectedShapeFor(paramDef, [{ path: 'x.ts' }])).toBe('[{"path":"x.ts"}]'); diff --git a/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts b/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts index 241f39415..84eb22d1a 100644 --- a/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts +++ b/tests/unit/gadgets/shared/promptExamplesGrammar.test.ts @@ -23,6 +23,7 @@ import { buildToolGuidance } from '../../../../src/backends/shared/nativeToolPro import * as githubDefs from '../../../../src/gadgets/github/definitions.js'; import { createPRReviewDef } from '../../../../src/gadgets/github/definitions.js'; import * as pmDefs from '../../../../src/gadgets/pm/definitions.js'; +import { readWorkItemDef } from '../../../../src/gadgets/pm/definitions.js'; import * as sentryDefs from '../../../../src/gadgets/sentry/definitions.js'; import * as sessionDefs from '../../../../src/gadgets/session/definitions.js'; import { generateToolManifest } from '../../../../src/gadgets/shared/manifestGenerator.js'; @@ -95,6 +96,16 @@ describe('prompt-rendered examples — CLI grammar correctness', () => { expect(rendered).not.toContain(`--event '"COMMENT"'`); }); + it('ReadWorkItem renders shell-safe PM IDs without literal quotes', () => { + const manifest = generateToolManifest(readWorkItemDef); + const rendered = buildToolGuidance([manifest]); + + expect(rendered).toContain('# example: --workItemId abc123'); + expect(rendered).not.toContain(`--workItemId '"abc123"'`); + expect(rendered).not.toContain(`--workItemId 'abc123'`); + expect(rendered).not.toContain(`--workItemId "abc123"`); + }); + it('CreatePRReview keeps array-of-object comments as JSON payload examples', () => { const manifest = generateToolManifest(createPRReviewDef); const rendered = buildToolGuidance([manifest]);