diff --git a/src/api/routers/pm-discovery.ts b/src/api/routers/pm-discovery.ts index db411167..7442d5db 100644 --- a/src/api/routers/pm-discovery.ts +++ b/src/api/routers/pm-discovery.ts @@ -15,20 +15,79 @@ import { TRPCError } from '@trpc/server'; import { z } from 'zod'; import { getIntegrationCredentialOrNull } from '../../config/provider.js'; import { getIntegrationByProjectAndCategory } from '../../db/repositories/integrationsRepository.js'; +import type { PMProviderManifest } from '../../integrations/pm/manifest.js'; import { getPMProvider, listPMProviders } from '../../integrations/pm/registry.js'; import { DISCOVERY_CAPABILITIES } from '../../pm/types.js'; import { protectedProcedure, router } from '../trpc.js'; import { verifyProjectOrgAccess } from './_shared/projectAccess.js'; +/** + * Invoke a manifest's optional `configToCredentials` hook and return the + * promoted bag. Guards against malformed hook returns and swallows hook + * errors with a warn so one broken provider cannot take down discovery + * for everyone. + */ +function promoteConfigCredentials( + manifest: PMProviderManifest, + integrationConfig: unknown, +): Record { + if (!manifest.configToCredentials) return {}; + try { + const promoted = manifest.configToCredentials(integrationConfig); + return promoted && typeof promoted === 'object' ? promoted : {}; + } catch (err) { + console.warn(`[pm-discovery] configToCredentials threw for provider '${manifest.id}':`, err); + return {}; + } +} + +/** + * Load + validate the PM integration for a given project. Throws the + * appropriate tRPC error when missing, misconfigured, or when the manifest + * has been deregistered. + */ +async function loadIntegrationAndManifest( + projectId: string, + providerId: string, +): Promise<{ integration: { config: unknown }; manifest: PMProviderManifest }> { + const integration = await getIntegrationByProjectAndCategory(projectId, 'pm'); + if (!integration) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: 'No PM integration configured for this project yet', + }); + } + if (integration.provider !== providerId) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: `Project is configured with a different PM provider (${integration.provider})`, + }); + } + const manifest = getPMProvider(providerId); + if (!manifest) { + throw new TRPCError({ + code: 'NOT_FOUND', + message: `Unknown PM provider '${providerId}'`, + }); + } + return { integration, manifest }; +} + /** * Shared credential resolver for pm.discovery.* endpoints. Accepts either * `credentials` directly or a `projectId` — if `projectId` is set, the * caller must have org access to the project, and we resolve each declared * credential role from the project_credentials table. * + * On the projectId path, the manifest's optional `configToCredentials` hook + * seeds the bag with non-secret connection fields promoted from + * `project_integrations.config` (e.g. JIRA's cloud tenant `baseUrl`). + * Values written from `project_credentials` override any key collisions — + * the DB-scoped secret always wins over config-derived defaults. + * * Returns a `Record` shaped by the manifest's - * `credentialRoles` — the shape downstream hooks / `createDiscoveryProvider` - * factories consume. + * `credentialRoles` (plus any promoted-config fields) — the shape + * downstream hooks / `createDiscoveryProvider` factories consume. */ async function resolvePMCredentials(opts: { providerId: string; @@ -36,44 +95,31 @@ async function resolvePMCredentials(opts: { credentials?: Record; projectId?: string; }): Promise> { - if (opts.projectId) { - if (!opts.effectiveOrgId) { - throw new TRPCError({ code: 'UNAUTHORIZED' }); - } - await verifyProjectOrgAccess(opts.projectId, opts.effectiveOrgId); - const integration = await getIntegrationByProjectAndCategory(opts.projectId, 'pm'); - if (!integration) { - throw new TRPCError({ - code: 'NOT_FOUND', - message: 'No PM integration configured for this project yet', - }); - } - if (integration.provider !== opts.providerId) { - throw new TRPCError({ - code: 'NOT_FOUND', - message: `Project is configured with a different PM provider (${integration.provider})`, - }); - } - const manifest = getPMProvider(opts.providerId); - if (!manifest) { - throw new TRPCError({ - code: 'NOT_FOUND', - message: `Unknown PM provider '${opts.providerId}'`, - }); - } - const resolved: Record = {}; - for (const role of manifest.credentialRoles) { - const value = await getIntegrationCredentialOrNull( - opts.projectId, - 'pm', - opts.providerId, - role.role, - ); - if (value) resolved[role.role] = value; - } - return resolved; + if (!opts.projectId) return opts.credentials ?? {}; + + if (!opts.effectiveOrgId) { + throw new TRPCError({ code: 'UNAUTHORIZED' }); + } + await verifyProjectOrgAccess(opts.projectId, opts.effectiveOrgId); + + const { integration, manifest } = await loadIntegrationAndManifest( + opts.projectId, + opts.providerId, + ); + + const resolved: Record = { + ...promoteConfigCredentials(manifest, integration.config), + }; + for (const role of manifest.credentialRoles) { + const value = await getIntegrationCredentialOrNull( + opts.projectId, + 'pm', + opts.providerId, + role.role, + ); + if (value) resolved[role.role] = value; } - return opts.credentials ?? {}; + return resolved; } const providerIdInput = z.object({ diff --git a/src/gadgets/github/core/createPR.ts b/src/gadgets/github/core/createPR.ts index b32e80e8..9b81f537 100644 --- a/src/gadgets/github/core/createPR.ts +++ b/src/gadgets/github/core/createPR.ts @@ -24,13 +24,14 @@ export interface CreatePRResult { commitOutput?: string; } -// Spec 013: per-caller timeouts for the two commands that trigger user-defined -// hooks. Values are sized to sit just under the gadget's 240s ceiling and to -// give test suites enough headroom for their slowest inter-event gaps. -const PUSH_WALL_TIMEOUT_MS = 230_000; -const PUSH_IDLE_TIMEOUT_MS = 90_000; -const COMMIT_WALL_TIMEOUT_MS = 120_000; -const COMMIT_IDLE_TIMEOUT_MS = 60_000; +// Timeouts are deliberately disabled on `git commit` and `git push`. Both +// commands invoke user-defined hooks (lefthook, husky, etc.) that can legitimately +// run full test suites for five-plus minutes. The agent harness that wraps this +// gadget handles long-running tool calls on its own, so a second shorter cap +// here would just re-introduce the "PUSH FAILED at 2 min" incident of spec 013. +// Heartbeat stays default (30s stderr pulse via runCommand), so operators still +// see `[git-push] still running (Ns)` ticks during slow hooks. Setting +// wallTimeoutMs + idleTimeoutMs to 0 disables them — see runCommand in utils/repo.ts. async function detectOwnerRepo(): Promise<{ owner: string; repo: string }> { const result = await runCommand('git', ['remote', 'get-url', 'origin'], process.cwd()); @@ -80,11 +81,7 @@ async function stageAndCommit(commitMessage: string): Promise { ['commit', '-m', commitMessage], process.cwd(), undefined, - { - label: 'git-commit', - wallTimeoutMs: COMMIT_WALL_TIMEOUT_MS, - idleTimeoutMs: COMMIT_IDLE_TIMEOUT_MS, - }, + { label: 'git-commit', wallTimeoutMs: 0, idleTimeoutMs: 0 }, ); if (commitResult.exitCode !== 0) { const output = [commitResult.stdout, commitResult.stderr].filter(Boolean).join('\n').trim(); @@ -105,11 +102,7 @@ async function pushBranch(branch: string): Promise { ['push', '-u', 'origin', branch], process.cwd(), undefined, - { - label: 'git-push', - wallTimeoutMs: PUSH_WALL_TIMEOUT_MS, - idleTimeoutMs: PUSH_IDLE_TIMEOUT_MS, - }, + { label: 'git-push', wallTimeoutMs: 0, idleTimeoutMs: 0 }, ); if (pushResult.exitCode !== 0) { const output = [pushResult.stdout, pushResult.stderr].filter(Boolean).join('\n').trim(); diff --git a/src/gadgets/github/definitions.ts b/src/gadgets/github/definitions.ts index eecedbfb..63c91fb5 100644 --- a/src/gadgets/github/definitions.ts +++ b/src/gadgets/github/definitions.ts @@ -48,8 +48,14 @@ The PR body supports full GitHub-flavored markdown including: - Tables NOTE: Pre-commit and pre-push hooks may run tests which can take time. -If hooks fail or timeout, the full output will be shown.`, - timeoutMs: 240000, // 4 minutes - hooks may run test suites +If hooks fail, the full output will be shown.`, + // Disabled: pre-commit / pre-push hooks can legitimately run a full test + // suite for 5+ minutes. The agent harness handles long-running tool calls + // on its own; `timeoutMs: 0` tells llmist not to arm an outer timer (see + // `if (timeoutMs && timeoutMs > 0)` in the dispatch path). runCommand's + // subprocess timeouts for `git commit` / `git push` are likewise disabled + // in core/createPR.ts. + timeoutMs: 0, parameters: { comment: { type: 'string', diff --git a/src/integrations/README.md b/src/integrations/README.md index 3b94150b..0bce39a6 100644 --- a/src/integrations/README.md +++ b/src/integrations/README.md @@ -58,6 +58,7 @@ See [`src/integrations/pm/manifest.ts`](./pm/manifest.ts) for the authoritative | `isSelfAuthoredHook?` | Optional — returns `true` when the event was authored by CASCADE itself (for loop prevention). | | `createLabel?` | Optional — enables the wizard's "Create label" button. Called via the generic `pm.discovery.createLabel` tRPC endpoint; signature is `({credentials, containerId, name, color?}) => {id, name, color}`. | | `createCustomField?` | Optional — enables wizard-driven custom-field creation. Called via `pm.discovery.createCustomField`; signature is `({credentials, containerId, name}) => {id, name, type}`. JIRA fields are global (the hook ignores containerId). | +| `configToCredentials?` | Optional — promotes non-secret connection fields from `project_integrations.config` into the credentials bag `createDiscoveryProvider` consumes. Signature: `(config: unknown) => Record`. Invoked only on the `projectId` path of `pm.discovery.*`; `project_credentials` values win on key collisions. Declare this when your provider stores tenant/host info in config instead of credentials (JIRA's `baseUrl` → `base_url`). Without it, edit-mode wizard re-verification constructs a client with empty host info — see prod incident 2026-04-24. | ### Plan 009 hardened-contract fields (all optional; providers opt in) diff --git a/src/integrations/pm/jira/manifest.ts b/src/integrations/pm/jira/manifest.ts index e59e40a1..3b479b3c 100644 --- a/src/integrations/pm/jira/manifest.ts +++ b/src/integrations/pm/jira/manifest.ts @@ -133,6 +133,22 @@ export const jiraManifest: PMProviderManifest = { ], }, + /** + * JIRA's cloud tenant URL is a non-secret connection field stored on + * `project_integrations.config.baseUrl`, not `project_credentials`. The + * pm-discovery resolver invokes this hook on the projectId path to + * promote the URL into the credentials bag the `createDiscoveryProvider` + * factory below consumes. Without it, edit-mode re-verification in the + * wizard constructs `new Version3Client({ host: '' })` and throws + * "Couldn't parse the host URL" (prod incident 2026-04-24). + */ + configToCredentials: (config: unknown): Record => { + if (!config || typeof config !== 'object') return {}; + const baseUrl = (config as { baseUrl?: unknown }).baseUrl; + if (typeof baseUrl !== 'string' || baseUrl.length === 0) return {}; + return { base_url: baseUrl }; + }, + configSchema: jiraConfigSchema, configFixture: { projectKey: 'CASCADE', diff --git a/src/integrations/pm/manifest.ts b/src/integrations/pm/manifest.ts index 877f4a2f..63eb7775 100644 --- a/src/integrations/pm/manifest.ts +++ b/src/integrations/pm/manifest.ts @@ -282,6 +282,28 @@ export interface PMProviderManifest { readonly createDiscoveryProvider?: (opts?: { credentials?: Record; }) => import('../../pm/types.js').PMProvider; + + /** + * Promote fields from the persisted integration config into the credentials + * bag that `createDiscoveryProvider` consumes. + * + * Motivation: some providers (JIRA) require non-secret connection fields — + * like the cloud tenant URL — that belong on `project_integrations.config`, + * not in `project_credentials`. Without this hook, `pm.discovery.discover` + * resolving credentials by projectId produces a bag missing those fields, + * and the discovery adapter constructs a client with an empty host (see + * prod incident 2026-04-24: "Couldn't parse the host URL" in the JIRA + * wizard's Select Project step). + * + * Contract: + * - Invoked only on the projectId path of `resolvePMCredentials`. The + * explicit-credentials path (wizard first-time setup, with the user's + * raw form values) does not invoke it. + * - Values loaded from `project_credentials` take precedence on key + * collisions — hook-returned values fill gaps, they don't override. + * - Return `{}` (or undefined) when the config has nothing to promote. + */ + readonly configToCredentials?: (config: unknown) => Record; } /** diff --git a/tests/unit/api/pm-discovery.test.ts b/tests/unit/api/pm-discovery.test.ts index a4e08bbc..d91bc041 100644 --- a/tests/unit/api/pm-discovery.test.ts +++ b/tests/unit/api/pm-discovery.test.ts @@ -25,6 +25,18 @@ vi.mock('../../../src/api/trpc.js', async () => { }; }); +// Mock the DB-bound helpers used by the projectId-credential-resolution path +// so tests can exercise that branch without a live database. +vi.mock('../../../src/db/repositories/integrationsRepository.js', () => ({ + getIntegrationByProjectAndCategory: vi.fn(), +})); +vi.mock('../../../src/config/provider.js', () => ({ + getIntegrationCredentialOrNull: vi.fn(), +})); +vi.mock('../../../src/api/routers/_shared/projectAccess.js', () => ({ + verifyProjectOrgAccess: vi.fn(async () => {}), +})); + import { pmDiscoveryRouter } from '../../../src/api/routers/pm-discovery.js'; import type { PMProviderManifest } from '../../../src/integrations/pm/manifest.js'; import { @@ -314,6 +326,355 @@ describe('pmDiscoveryRouter', () => { }); }); + describe('configToCredentials — projectId-path config promotion', () => { + // Regression for the 2026-04-24 production bug: a saved JIRA integration's + // base URL lives on `project_integrations.config.baseUrl`, not on + // `project_credentials`. `pm.discovery.discover({ projectId })` therefore + // constructed a JIRA client with `host: ''` → "Couldn't parse the host URL". + // The fix is a `configToCredentials` manifest hook that promotes specific + // integration-config fields into the credentials bag used by + // `createDiscoveryProvider`. + + beforeEach(() => { + _resetPMProviderRegistryForTesting(); + vi.clearAllMocks(); + }); + + it('merges config-derived credentials from configToCredentials into the resolved bag', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const base = createFakePMManifest(); + + let receivedCredentials: Record | undefined; + registerPMProvider({ + ...base, + id: 'fake-with-config-creds', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + configToCredentials: (config: unknown) => { + const c = config as { tenantUrl?: string }; + return c.tenantUrl ? { base_url: c.tenantUrl } : {}; + }, + createDiscoveryProvider: (opts) => { + receivedCredentials = opts?.credentials ?? {}; + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + const { getIntegrationCredentialOrNull } = await import('../../../src/config/provider.js'); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'ua-store', + category: 'pm', + provider: 'fake-with-config-creds', + config: { tenantUrl: 'https://example.atlassian.net' }, + triggers: {}, + } as unknown as Awaited>); + vi.mocked(getIntegrationCredentialOrNull).mockResolvedValue('secret-key'); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-with-config-creds', + capability: 'currentUser', + args: {}, + projectId: 'ua-store', + }); + + expect(receivedCredentials).toEqual({ + api_key: 'secret-key', + base_url: 'https://example.atlassian.net', + }); + }); + + it('project_credentials values win over config-derived values on collision', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const base = createFakePMManifest(); + + let receivedCredentials: Record | undefined; + registerPMProvider({ + ...base, + id: 'fake-collision', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + // Intentionally overlaps with credentialRoles to assert precedence. + configToCredentials: () => ({ api_key: 'from-config' }), + createDiscoveryProvider: (opts) => { + receivedCredentials = opts?.credentials ?? {}; + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + const { getIntegrationCredentialOrNull } = await import('../../../src/config/provider.js'); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'p', + category: 'pm', + provider: 'fake-collision', + config: {}, + triggers: {}, + } as unknown as Awaited>); + vi.mocked(getIntegrationCredentialOrNull).mockResolvedValue('from-db'); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-collision', + capability: 'currentUser', + args: {}, + projectId: 'p', + }); + + expect(receivedCredentials).toEqual({ api_key: 'from-db' }); + }); + + it('manifests without configToCredentials still work (legacy behavior preserved)', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const base = createFakePMManifest(); + + let receivedCredentials: Record | undefined; + registerPMProvider({ + ...base, + id: 'fake-no-hook', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + configToCredentials: undefined, + createDiscoveryProvider: (opts) => { + receivedCredentials = opts?.credentials ?? {}; + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + const { getIntegrationCredentialOrNull } = await import('../../../src/config/provider.js'); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'p', + category: 'pm', + provider: 'fake-no-hook', + config: { tenantUrl: 'https://example.atlassian.net' }, + triggers: {}, + } as unknown as Awaited>); + vi.mocked(getIntegrationCredentialOrNull).mockResolvedValue('secret-key'); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-no-hook', + capability: 'currentUser', + args: {}, + projectId: 'p', + }); + + expect(receivedCredentials).toEqual({ api_key: 'secret-key' }); + }); + + it('does not invoke configToCredentials on the explicit-credentials path', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const base = createFakePMManifest(); + + const hookSpy = vi.fn(() => ({ base_url: 'should-not-appear' })); + registerPMProvider({ + ...base, + id: 'fake-no-projectid', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + configToCredentials: hookSpy, + createDiscoveryProvider: () => { + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-no-projectid', + capability: 'currentUser', + args: {}, + credentials: { api_key: 'k' }, + }); + + expect(hookSpy).not.toHaveBeenCalled(); + }); + + // ── Error paths introduced by the resolvePMCredentials refactor ───── + // The projectId branch now flows through two new helpers + // (promoteConfigCredentials + loadIntegrationAndManifest) with several + // guard throws. These tests pin each branch so a future refactor + // cannot silently drop one. + + it('throws UNAUTHORIZED when projectId is set but effectiveOrgId is null', async () => { + const { createFakePMManifest } = await import('../../helpers/fakePMProvider.js'); + registerPMProvider({ ...createFakePMManifest(), id: 'fake-auth' }); + + const caller = pmDiscoveryRouter.createCaller({ + effectiveOrgId: null as unknown as string, + }); + await expect( + caller.discover({ + providerId: 'fake-auth', + capability: 'currentUser', + args: {}, + projectId: 'some-project', + }), + ).rejects.toMatchObject({ code: 'UNAUTHORIZED' }); + }); + + it('throws NOT_FOUND when the project has no PM integration configured', async () => { + const { createFakePMManifest } = await import('../../helpers/fakePMProvider.js'); + registerPMProvider({ ...createFakePMManifest(), id: 'fake-missing' }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue(null); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-missing', + capability: 'currentUser', + args: {}, + projectId: 'orphan-project', + }), + ).rejects.toMatchObject({ + code: 'NOT_FOUND', + message: expect.stringMatching(/No PM integration/i), + }); + }); + + it('throws NOT_FOUND when the saved integration is for a different provider', async () => { + const { createFakePMManifest } = await import('../../helpers/fakePMProvider.js'); + registerPMProvider({ ...createFakePMManifest(), id: 'fake-expected' }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'p', + category: 'pm', + provider: 'fake-other', + config: {}, + triggers: {}, + } as unknown as Awaited>); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await expect( + caller.discover({ + providerId: 'fake-expected', + capability: 'currentUser', + args: {}, + projectId: 'p', + }), + ).rejects.toMatchObject({ + code: 'NOT_FOUND', + message: expect.stringMatching(/different PM provider.*fake-other/), + }); + }); + + it('treats a non-object hook return (string/null/array) as empty — resolved bag contains only project_credentials', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + + let receivedCredentials: Record | undefined; + registerPMProvider({ + ...createFakePMManifest(), + id: 'fake-bad-hook-return', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + // Hook returns a non-object: must be ignored, must not crash. + configToCredentials: () => 'not-an-object' as unknown as Record, + createDiscoveryProvider: (opts) => { + receivedCredentials = opts?.credentials ?? {}; + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + const { getIntegrationCredentialOrNull } = await import('../../../src/config/provider.js'); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'p', + category: 'pm', + provider: 'fake-bad-hook-return', + config: {}, + triggers: {}, + } as unknown as Awaited>); + vi.mocked(getIntegrationCredentialOrNull).mockResolvedValue('k'); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-bad-hook-return', + capability: 'currentUser', + args: {}, + projectId: 'p', + }); + + expect(receivedCredentials).toEqual({ api_key: 'k' }); + }); + + it('swallows hook exceptions and continues with project_credentials (logs a warn)', async () => { + const { createFakePMManifest, createFakePMProvider } = await import( + '../../helpers/fakePMProvider.js' + ); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + let receivedCredentials: Record | undefined; + registerPMProvider({ + ...createFakePMManifest(), + id: 'fake-throwing-hook', + credentialRoles: [{ role: 'api_key', label: 'API Key', envVarKey: 'FAKE_API_KEY' }], + // A broken hook MUST NOT take down discovery. + configToCredentials: () => { + throw new Error('hook boom'); + }, + createDiscoveryProvider: (opts) => { + receivedCredentials = opts?.credentials ?? {}; + const { provider } = createFakePMProvider(); + return provider; + }, + }); + + const { getIntegrationByProjectAndCategory } = await import( + '../../../src/db/repositories/integrationsRepository.js' + ); + const { getIntegrationCredentialOrNull } = await import('../../../src/config/provider.js'); + vi.mocked(getIntegrationByProjectAndCategory).mockResolvedValue({ + projectId: 'p', + category: 'pm', + provider: 'fake-throwing-hook', + config: {}, + triggers: {}, + } as unknown as Awaited>); + vi.mocked(getIntegrationCredentialOrNull).mockResolvedValue('k'); + + const caller = pmDiscoveryRouter.createCaller({ effectiveOrgId: 'org-1' }); + await caller.discover({ + providerId: 'fake-throwing-hook', + capability: 'currentUser', + args: {}, + projectId: 'p', + }); + + expect(receivedCredentials).toEqual({ api_key: 'k' }); + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining("configToCredentials threw for provider 'fake-throwing-hook'"), + expect.any(Error), + ); + warnSpy.mockRestore(); + }); + }); + describe('createCustomField (plan 010/1 task 2)', () => { beforeEach(async () => { _resetPMProviderRegistryForTesting(); diff --git a/tests/unit/gadgets/github/core/createPR.test.ts b/tests/unit/gadgets/github/core/createPR.test.ts index 74b3c996..e46b2ee6 100644 --- a/tests/unit/gadgets/github/core/createPR.test.ts +++ b/tests/unit/gadgets/github/core/createPR.test.ts @@ -529,13 +529,23 @@ describe('createPR', () => { }); // ──────────────────────────────────────────────────────────────────────────── -// Spec 013: per-caller timeouts + captured hook output preservation +// Spec 013: captured hook output preservation +// (The per-caller timeout assertions that used to live here were removed +// together with the timeouts they pinned — pre-commit / pre-push hooks can +// legitimately run for minutes and the harness handles long calls. The new +// "regression: stay disabled" block below pins the current contract.) // ──────────────────────────────────────────────────────────────────────────── -describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { - it('pushBranch passes an explicit wallTimeoutMs below the gadget 240s ceiling', async () => { +describe('captured hook output preservation (spec 013)', () => { + it('createPR result carries captured push output on success', async () => { mockRunCommand.mockImplementation(async (_cmd, args) => { if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'push') + return { + stdout: 'Pre-push hook ran: typecheck OK\n', + stderr: 'To github.com...\n', + exitCode: 0, + }; if (args?.[0] === 'ls-remote') return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; return { stdout: '', stderr: '', exitCode: 0 }; @@ -545,7 +555,7 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', } as Awaited>); - await createPR({ + const result = await createPR({ title: 'Test', body: 'Body', head: 'feat', @@ -554,20 +564,20 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { push: true, }); - const pushCall = mockRunCommand.mock.calls.find( - (c) => Array.isArray(c[1]) && c[1][0] === 'push', - ); - expect(pushCall).toBeDefined(); - // 5th arg is RunCommandOptions - const options = pushCall?.[4] as { wallTimeoutMs?: number } | undefined; - expect(options).toBeDefined(); - expect(typeof options?.wallTimeoutMs).toBe('number'); - expect(options?.wallTimeoutMs).toBeLessThanOrEqual(230_000); + expect(result.pushOutput).toBeDefined(); + expect(result.pushOutput).toContain('Pre-push hook ran: typecheck OK'); }); - it('pushBranch passes an explicit finite idleTimeoutMs', async () => { + it('createPR result carries captured commit output on success', async () => { mockRunCommand.mockImplementation(async (_cmd, args) => { if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + if (args?.[0] === 'status') return { stdout: 'M foo.ts\n', stderr: '', exitCode: 0 }; + if (args?.[0] === 'commit') + return { + stdout: 'Pre-commit hook ran: biome OK\n[feat abc123] msg\n', + stderr: '', + exitCode: 0, + }; if (args?.[0] === 'ls-remote') return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; return { stdout: '', stderr: '', exitCode: 0 }; @@ -577,26 +587,39 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', } as Awaited>); - await createPR({ + const result = await createPR({ title: 'Test', body: 'Body', head: 'feat', base: 'main', - commit: false, - push: true, + commit: true, + push: false, }); - const pushCall = mockRunCommand.mock.calls.find( - (c) => Array.isArray(c[1]) && c[1][0] === 'push', - ); - const options = pushCall?.[4] as { idleTimeoutMs?: number } | undefined; - expect(typeof options?.idleTimeoutMs).toBe('number'); - expect(options?.idleTimeoutMs).toBeGreaterThan(0); + expect(result.commitOutput).toBeDefined(); + expect(result.commitOutput).toContain('Pre-commit hook ran: biome OK'); }); +}); - it('stageAndCommit passes explicit wallTimeoutMs and idleTimeoutMs', async () => { - mockRunCommand.mockImplementation(async (_cmd, args) => { - if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; +// ─────────────────────────────────────────────────────────────────────────── +// Regression: subprocess timeouts on `git commit` and `git push` must stay +// disabled. Pre-commit / pre-push hooks legitimately run for minutes (full +// test suites), and the agent harness that wraps this gadget already budgets +// long calls. Re-introducing a shorter cap here is the exact failure mode +// that burned an earlier session — do not "just add a safety cap". +// ─────────────────────────────────────────────────────────────────────────── +describe('subprocess timeouts — regression: stay disabled on hook-invoking calls', () => { + function runCommandCallFor(args: string[]) { + // Find the call whose args[0] matches (e.g. 'commit', 'push'). + return mockRunCommand.mock.calls.find((call) => { + const callArgs = call[1]; + return Array.isArray(callArgs) && callArgs[0] === args[0]; + }); + } + + it('git commit is invoked with wallTimeoutMs=0 and idleTimeoutMs=0', async () => { + mockRunCommand.mockReset(); + mockGitCommands((_cmd, args) => { if (args?.[0] === 'status') return { stdout: 'M foo.ts\n', stderr: '', exitCode: 0 }; if (args?.[0] === 'ls-remote') return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; @@ -616,26 +639,20 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { push: false, }); - const commitCall = mockRunCommand.mock.calls.find( - (c) => Array.isArray(c[1]) && c[1][0] === 'commit', - ); + const commitCall = runCommandCallFor(['commit']); expect(commitCall).toBeDefined(); - const options = commitCall?.[4] as - | { wallTimeoutMs?: number; idleTimeoutMs?: number } - | undefined; - expect(typeof options?.wallTimeoutMs).toBe('number'); - expect(typeof options?.idleTimeoutMs).toBe('number'); + // Positional arg 4 (0-indexed) is the RunCommandOptions bag. + const opts = commitCall?.[4]; + expect(opts).toMatchObject({ + label: 'git-commit', + wallTimeoutMs: 0, + idleTimeoutMs: 0, + }); }); - it('createPR result carries captured push output on success', async () => { - mockRunCommand.mockImplementation(async (_cmd, args) => { - if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; - if (args?.[0] === 'push') - return { - stdout: 'Pre-push hook ran: typecheck OK\n', - stderr: 'To github.com...\n', - exitCode: 0, - }; + it('git push is invoked with wallTimeoutMs=0 and idleTimeoutMs=0', async () => { + mockRunCommand.mockReset(); + mockGitCommands((_cmd, args) => { if (args?.[0] === 'ls-remote') return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; return { stdout: '', stderr: '', exitCode: 0 }; @@ -645,7 +662,7 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', } as Awaited>); - const result = await createPR({ + await createPR({ title: 'Test', body: 'Body', head: 'feat', @@ -654,20 +671,23 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { push: true, }); - expect(result.pushOutput).toBeDefined(); - expect(result.pushOutput).toContain('Pre-push hook ran: typecheck OK'); + const pushCall = runCommandCallFor(['push']); + expect(pushCall).toBeDefined(); + const opts = pushCall?.[4]; + expect(opts).toMatchObject({ + label: 'git-push', + wallTimeoutMs: 0, + idleTimeoutMs: 0, + }); }); - it('createPR result carries captured commit output on success', async () => { - mockRunCommand.mockImplementation(async (_cmd, args) => { - if (args?.[0] === 'remote') return { stdout: HTTPS_URL, stderr: '', exitCode: 0 }; + it('does not reference any legacy per-caller timeout constant name in runCommand options', async () => { + // Sentinel against silent regression: if someone re-adds + // PUSH_WALL_TIMEOUT_MS / COMMIT_IDLE_TIMEOUT_MS etc., they will likely + // pass a non-zero number here. Zero is the only accepted value. + mockRunCommand.mockReset(); + mockGitCommands((_cmd, args) => { if (args?.[0] === 'status') return { stdout: 'M foo.ts\n', stderr: '', exitCode: 0 }; - if (args?.[0] === 'commit') - return { - stdout: 'Pre-commit hook ran: biome OK\n[feat abc123] msg\n', - stderr: '', - exitCode: 0, - }; if (args?.[0] === 'ls-remote') return { stdout: 'abc\trefs/heads/feat', stderr: '', exitCode: 0 }; return { stdout: '', stderr: '', exitCode: 0 }; @@ -677,16 +697,23 @@ describe('pushBranch and stageAndCommit timeout options (spec 013)', () => { htmlUrl: 'https://github.com/test-owner/test-repo/pull/1', } as Awaited>); - const result = await createPR({ + await createPR({ title: 'Test', body: 'Body', head: 'feat', base: 'main', commit: true, - push: false, + push: true, }); - expect(result.commitOutput).toBeDefined(); - expect(result.commitOutput).toContain('Pre-commit hook ran: biome OK'); + for (const needle of ['commit', 'push']) { + const call = runCommandCallFor([needle]); + const opts = (call?.[4] ?? {}) as { + wallTimeoutMs?: number; + idleTimeoutMs?: number; + }; + expect(opts.wallTimeoutMs, `${needle} wallTimeoutMs must be 0`).toBe(0); + expect(opts.idleTimeoutMs, `${needle} idleTimeoutMs must be 0`).toBe(0); + } }); }); diff --git a/tests/unit/gadgets/github/definitions.test.ts b/tests/unit/gadgets/github/definitions.test.ts index b133349d..33a89db1 100644 --- a/tests/unit/gadgets/github/definitions.test.ts +++ b/tests/unit/gadgets/github/definitions.test.ts @@ -52,10 +52,14 @@ describe('GitHub SCM gadget definitions', () => { } }); - it('every definition has a timeoutMs greater than 0', () => { + it('every definition has a non-negative timeoutMs (0 is the "disabled" sentinel)', () => { + // `timeoutMs: 0` is a deliberate opt-out for long-running tools whose + // runtime is already managed externally (e.g. CreatePR — pre-commit / + // pre-push hooks can take minutes, and the agent harness surrounds the + // call with its own budget). Anything negative is definitively a bug. for (const def of ALL_SCM_DEFINITIONS) { if (def.timeoutMs !== undefined) { - expect(def.timeoutMs).toBeGreaterThan(0); + expect(def.timeoutMs).toBeGreaterThanOrEqual(0); } } }); @@ -168,8 +172,12 @@ describe('GitHub SCM gadget definitions', () => { expect((createPRDef.parameters.push as { default?: boolean })?.default).toBe(true); }); - it('has a 4-minute timeout (hooks may run test suites)', () => { - expect(createPRDef.timeoutMs).toBe(240000); + it('has timeoutMs disabled (0) — pre-commit/pre-push hooks may run for minutes and the harness handles long calls', () => { + // Regression guard: do not reintroduce an outer time cap here without + // first considering that legitimate pre-push hooks (full test suites) + // can run five-plus minutes. A shorter cap here turned into a + // production incident once (see chore: remove-createpr-timeouts). + expect(createPRDef.timeoutMs).toBe(0); }); it('has body file input alternative in CLI', () => { diff --git a/tests/unit/pm/jira/manifest-config-to-credentials.test.ts b/tests/unit/pm/jira/manifest-config-to-credentials.test.ts new file mode 100644 index 00000000..a212ad58 --- /dev/null +++ b/tests/unit/pm/jira/manifest-config-to-credentials.test.ts @@ -0,0 +1,72 @@ +/** + * JIRA manifest `configToCredentials` hook. + * + * Regression pin for the 2026-04-24 prod incident: the JIRA wizard's + * Select Project step returned "Error: Internal server error" because + * `pm.discovery.discover({ projectId })` resolved credentials only from + * `project_credentials` (email, api_token) and never read `baseUrl` off + * the integration config, so the JIRA client constructed + * `new Version3Client({ host: '' })` and threw "Couldn't parse the host URL". + * + * The fix is a manifest-level `configToCredentials` hook that promotes + * `config.baseUrl` into the discovery credentials bag as `base_url`. + * These tests pin that contract at the manifest layer, independent of + * the router wiring (which is covered by the pm-discovery router test). + */ + +import { beforeAll, describe, expect, it } from 'vitest'; +import { jiraManifest } from '../../../../src/integrations/pm/jira/manifest.js'; + +describe('jiraManifest.configToCredentials', () => { + let hook: NonNullable; + + beforeAll(() => { + const declared = jiraManifest.configToCredentials; + if (!declared) { + throw new Error( + 'jiraManifest.configToCredentials must be declared — the 2026-04-24 wizard incident depends on it.', + ); + } + hook = declared; + }); + + it('promotes baseUrl from config into base_url credential slot', () => { + expect(hook({ baseUrl: 'https://acme.atlassian.net' })).toEqual({ + base_url: 'https://acme.atlassian.net', + }); + }); + + it('returns an empty object when baseUrl is missing', () => { + expect(hook({})).toEqual({}); + }); + + it('returns an empty object when baseUrl is an empty string', () => { + expect(hook({ baseUrl: '' })).toEqual({}); + }); + + it('returns an empty object when baseUrl is non-string', () => { + expect(hook({ baseUrl: 123 })).toEqual({}); + expect(hook({ baseUrl: null })).toEqual({}); + expect(hook({ baseUrl: { nested: 'x' } })).toEqual({}); + }); + + it('returns an empty object when config is null or undefined', () => { + expect(hook(null)).toEqual({}); + expect(hook(undefined)).toEqual({}); + }); + + it('returns an empty object when config is not an object', () => { + expect(hook('string-config')).toEqual({}); + expect(hook(42)).toEqual({}); + }); + + it('ignores unrelated config fields', () => { + expect( + hook({ + baseUrl: 'https://acme.atlassian.net', + projectKey: 'CASCADE', + statuses: { todo: 'To Do' }, + }), + ).toEqual({ base_url: 'https://acme.atlassian.net' }); + }); +}); diff --git a/tests/unit/utils/repo.test.ts b/tests/unit/utils/repo.test.ts index c6949094..cfcf56a0 100644 --- a/tests/unit/utils/repo.test.ts +++ b/tests/unit/utils/repo.test.ts @@ -418,6 +418,73 @@ describe('repo utils', () => { await promise; }); + // ─── Regression: wallTimeoutMs / idleTimeoutMs set to 0 must disable ── + // Spec contract at `runCommand` docstring: "Setting a timing field to 0 + // disables it." Callers like CreatePR rely on this to keep pre-push + // hooks alive for minutes. A regression that re-enables the timer when + // 0 is passed would silently bring back the "PUSH FAILED at 2 min" + // incident that motivated removing the caps in the first place. + + it('does NOT kill the child when idleTimeoutMs is 0 — even after 10 min of silence', async () => { + vi.useFakeTimers(); + const child = createMockSubprocess(); + vi.mocked(execa).mockReturnValue(child as unknown as ReturnType); + + const promise = runCommand('long-cmd', [], '/tmp', undefined, { + idleTimeoutMs: 0, + heartbeatMs: 0, + wallTimeoutMs: 0, + }); + await Promise.resolve(); + + // 10 minutes of silence — would trip any armed idle timer (default 2m). + vi.advanceTimersByTime(10 * 60 * 1000); + await Promise.resolve(); + + expect(vi.mocked(treeKill)).not.toHaveBeenCalled(); + + // Settle successfully. + child.stdout.push(null); + child.stderr.push(null); + child.resolveExec({ stdout: 'done\n', stderr: '', exitCode: 0 }); + vi.useRealTimers(); + const result = await promise; + expect(result.reason).toBeUndefined(); + expect(result.exitCode).toBe(0); + }); + + it('does NOT kill the child when wallTimeoutMs is 0 — even after 10 min of continuous output', async () => { + vi.useFakeTimers(); + const child = createMockSubprocess(); + vi.mocked(execa).mockReturnValue(child as unknown as ReturnType); + + const promise = runCommand('long-cmd', [], '/tmp', undefined, { + wallTimeoutMs: 0, + idleTimeoutMs: 0, + heartbeatMs: 0, + }); + await Promise.resolve(); + + // 10 minutes of constant chatter — would blow past the default 10m wall + // and the spec-013 230s cap, and would trip any armed wall timer. + for (let t = 0; t < 10 * 60 * 1000; t += 1000) { + child.stdout.push(`tick ${t}\n`); + await Promise.resolve(); + vi.advanceTimersByTime(1000); + await Promise.resolve(); + } + + expect(vi.mocked(treeKill)).not.toHaveBeenCalled(); + + child.stdout.push(null); + child.stderr.push(null); + child.resolveExec({ stdout: '', stderr: '', exitCode: 0 }); + vi.useRealTimers(); + const result = await promise; + expect(result.reason).toBeUndefined(); + expect(result.exitCode).toBe(0); + }); + it('kills the child via tree-kill with SIGTERM when wallTimeoutMs elapses even with ongoing output', async () => { vi.useFakeTimers(); const child = createMockSubprocess();