diff --git a/src/github/personas.ts b/src/github/personas.ts index e9b5708c..588acbf3 100644 --- a/src/github/personas.ts +++ b/src/github/personas.ts @@ -53,13 +53,32 @@ export async function getPersonaToken(projectId: string, agentType: string): Pro // Identity Resolution // ============================================================================ +const PERSONA_CACHE_TTL_MS = 60_000; // 60 seconds — matches the Trello/JIRA BotIdentityCache TTL + +interface CacheEntry { + value: PersonaIdentities; + expiresAt: number; +} + +// Per-project TTL cache for persona identities. +// Unlike BotIdentityCache, errors are re-thrown so callers retain error semantics. +const personaIdentityCache = new Map(); + /** * Resolve both persona GitHub usernames for a project. - * Always queries the database and GitHub API for fresh data. + * Results are cached per-project with a 60s TTL to avoid redundant DB + API calls + * on rapid successive webhooks (e.g. multiple events within the same request batch). + * Errors are re-thrown so callers can handle credential failures. */ export async function resolvePersonaIdentities(projectId: string): Promise { - const implementerToken = await getIntegrationCredential(projectId, 'scm', 'implementer_token'); - const reviewerToken = await getIntegrationCredential(projectId, 'scm', 'reviewer_token'); + const cached = personaIdentityCache.get(projectId); + if (cached && Date.now() < cached.expiresAt) return cached.value; + + // Parallelize credential lookups to halve round-trip latency + const [implementerToken, reviewerToken] = await Promise.all([ + getIntegrationCredential(projectId, 'scm', 'implementer_token'), + getIntegrationCredential(projectId, 'scm', 'reviewer_token'), + ]); const [implementerLogin, reviewerLogin] = await Promise.all([ getGitHubUserForToken(implementerToken), @@ -88,9 +107,18 @@ export async function resolvePersonaIdentities(projectId: string): Promise { - const resolved = await resolveGitHubTokenForAckByAgent(repoFullName, agentType); + const resolved = await resolveGitHubTokenForAckByAgent(repoFullName, agentType, project); if (!resolved) return undefined; const context = extractGitHubContext(payload, eventType); @@ -117,6 +119,45 @@ function getGitHubDeliveryId(payload: Record): string | undefin export class GitHubRouterAdapter implements RouterPlatformAdapter { readonly type = 'github' as const; + // ------------------------------------------------------------------------- + // Per-request caches + // + // GitHubRouterAdapter is instantiated once per webhook request, so instance + // fields are naturally request-scoped. Caching here avoids redundant DB and + // GitHub API calls across the pipeline steps (isSelfAuthored, sendReaction, + // dispatchWithCredentials, postAck) that all need the same project / persona + // data for a single event. + // ------------------------------------------------------------------------- + + private cachedProject: ProjectConfig | null | undefined = undefined; // undefined = not yet fetched + private cachedPersonas: PersonaIdentities | undefined; + private cachedFullProject: ProjectConfig | undefined; + + /** + * Resolve the ProjectConfig for the given repo, caching the result for the + * lifetime of this request adapter instance. + */ + private async findProjectCached(repoFullName: string): Promise { + if (this.cachedProject !== undefined) return this.cachedProject; + const project = (await findProjectByRepo(repoFullName)) ?? null; + this.cachedProject = project; + return project; + } + + /** + * Resolve persona identities for the given project, caching the result for + * the lifetime of this request adapter instance. + */ + private async resolvePersonaCached(projectId: string): Promise { + if (this.cachedPersonas !== undefined) return this.cachedPersonas; + try { + this.cachedPersonas = await resolvePersonaIdentities(projectId); + } catch { + // Resolution may fail — leave cachedPersonas undefined so callers can decide + } + return this.cachedPersonas; + } + async parseWebhook(payload: unknown): Promise { const p = payload as Record; const repo = p.repository as Record | undefined; @@ -161,9 +202,10 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { const login = commentUser?.login as string | undefined; if (!login) return false; try { - const project = await findProjectByRepo((event as GitHubParsedEvent).repoFullName); + const project = await this.findProjectCached((event as GitHubParsedEvent).repoFullName); if (!project) return false; - const personas = await resolvePersonaIdentities(project.id); + const personas = await this.resolvePersonaCached(project.id); + if (!personas) return false; return isCascadeBot(login, personas); } catch { return false; @@ -175,12 +217,16 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { const repoFullName = (event as GitHubParsedEvent).repoFullName; void (async () => { try { - const project = await findProjectByRepo(repoFullName); + const project = await this.findProjectCached(repoFullName); if (!project) { logger.warn('No project found for repo, skipping GitHub reaction', { repoFullName }); return; } - const personaIdentities = await resolvePersonaIdentities(project.id); + const personaIdentities = await this.resolvePersonaCached(project.id); + if (!personaIdentities) { + logger.warn('No persona identities resolved, skipping GitHub reaction', { repoFullName }); + return; + } await sendAcknowledgeReaction('github', repoFullName, payload, personaIdentities, project); } catch (err) { logger.warn('GitHub reaction error', { error: String(err), repoFullName }); @@ -215,12 +261,11 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { return null; } - let personaIdentities: PersonaIdentities | undefined; - try { - personaIdentities = await resolvePersonaIdentities(fullProject.id); - } catch { - // Persona resolution may fail — proceed without - } + // Cache fullProject for reuse in postAck (Step 4) + this.cachedFullProject = fullProject; + + // Reuse cached personas to avoid a second resolvePersonaIdentities call (Step 4) + const personaIdentities = await this.resolvePersonaCached(fullProject.id); const githubToken = await getProjectGitHubToken(fullProject); const pmProvider = pmRegistry.createProvider(fullProject); @@ -251,9 +296,13 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { triggerResult?: TriggerResult, ): Promise { try { - // Load full project config to check run link settings - const config = await loadProjectConfig(); - const fullProject = config.fullProjects.find((fp) => fp.id === project.id); + // Reuse the fullProject already resolved in dispatchWithCredentials when available + // (Step 4: avoids a second loadProjectConfig() call per webhook pipeline) + let fullProject = this.cachedFullProject; + if (!fullProject) { + const config = await loadProjectConfig(); + fullProject = config.fullProjects.find((fp) => fp.id === project.id); + } const runLinksEnabled = fullProject?.runLinksEnabled ?? false; // Helper to append run link footer to a message when enabled @@ -296,6 +345,7 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { } } + // Pass cached project to avoid a redundant findProjectByRepo() in the token resolver return postGitHubPRAck( (event as GitHubParsedEvent).repoFullName, event.eventType, @@ -303,6 +353,7 @@ export class GitHubRouterAdapter implements RouterPlatformAdapter { agentType, project.id, githubAckMessage, + fullProject, ); } catch (err) { logger.warn('GitHub ack comment failed (non-fatal)', { error: String(err) }); diff --git a/src/router/github-token-resolver.ts b/src/router/github-token-resolver.ts index d82c6bed..4866919a 100644 --- a/src/router/github-token-resolver.ts +++ b/src/router/github-token-resolver.ts @@ -43,21 +43,26 @@ export async function resolveGitHubTokenForAck( * is posted by the same persona that will run the agent (and can * later update it via ProgressMonitor). All other agents use the * implementer token. + * + * @param project — Optional pre-resolved project config. When provided, the + * `findProjectByRepo()` DB lookup is skipped entirely (eliminating a + * redundant query in callers that have already resolved the project). */ export async function resolveGitHubTokenForAckByAgent( repoFullName: string, agentType: string, + project?: ProjectConfig, ): Promise { - const project = await findProjectByRepo(repoFullName); - if (!project) return null; + const resolvedProject = project ?? (await findProjectByRepo(repoFullName)); + if (!resolvedProject) return null; try { if (agentType === 'review') { - const token = await getIntegrationCredential(project.id, 'scm', 'reviewer_token'); - return { token, project }; + const token = await getIntegrationCredential(resolvedProject.id, 'scm', 'reviewer_token'); + return { token, project: resolvedProject }; } - const token = await getProjectGitHubToken(project); - return { token, project }; + const token = await getProjectGitHubToken(resolvedProject); + return { token, project: resolvedProject }; } catch { logger.warn('[Ack] Missing GitHub token for repo:', repoFullName); return null; diff --git a/tests/unit/github/personas.test.ts b/tests/unit/github/personas.test.ts index 7e669060..1b54958f 100644 --- a/tests/unit/github/personas.test.ts +++ b/tests/unit/github/personas.test.ts @@ -21,6 +21,7 @@ vi.mock('../../../src/utils/logging.js', () => ({ import { getIntegrationCredential } from '../../../src/config/provider.js'; import { getGitHubUserForToken } from '../../../src/github/client.js'; import { + _resetPersonaIdentityCache, getPersonaForAgentType, getPersonaForLogin, getPersonaToken, @@ -30,6 +31,15 @@ import { import type { PersonaIdentities } from '../../../src/github/personas.js'; describe('personas', () => { + beforeEach(() => { + // Reset the module-level TTL cache so tests don't bleed into each other + _resetPersonaIdentityCache(); + }); + + afterEach(() => { + _resetPersonaIdentityCache(); + }); + // ======================================================================== // getPersonaForAgentType // ======================================================================== @@ -130,15 +140,11 @@ describe('personas', () => { }); }); - it('fetches fresh data on each call', async () => { + it('returns cached result on second call for same project', async () => { vi.mocked(getIntegrationCredential) - .mockResolvedValueOnce('ghp_impl') - .mockResolvedValueOnce('ghp_review') .mockResolvedValueOnce('ghp_impl') .mockResolvedValueOnce('ghp_review'); vi.mocked(getGitHubUserForToken) - .mockResolvedValueOnce('cascade-impl-bot') - .mockResolvedValueOnce('cascade-review-bot') .mockResolvedValueOnce('cascade-impl-bot') .mockResolvedValueOnce('cascade-review-bot'); @@ -146,7 +152,9 @@ describe('personas', () => { const second = await resolvePersonaIdentities('project1'); expect(first).toEqual(second); - expect(getIntegrationCredential).toHaveBeenCalledTimes(4); + // Only 2 credential lookups total (not 4) because the second call is served from cache + expect(getIntegrationCredential).toHaveBeenCalledTimes(2); + expect(getGitHubUserForToken).toHaveBeenCalledTimes(2); }); it('resolves separately for different projects', async () => { @@ -217,6 +225,102 @@ describe('personas', () => { 'Failed to resolve GitHub identity for reviewer token', ); }); + + it('fetches fresh data after TTL expires', async () => { + vi.useFakeTimers(); + const now = Date.now(); + + try { + // First call — populate the cache + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl') + .mockResolvedValueOnce('ghp_review'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('cascade-impl-bot') + .mockResolvedValueOnce('cascade-review-bot'); + + await resolvePersonaIdentities('project1'); + expect(getIntegrationCredential).toHaveBeenCalledTimes(2); + + // Advance time past the 60s TTL + vi.setSystemTime(now + 61_000); + + // Second call after TTL — should re-fetch + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl_new') + .mockResolvedValueOnce('ghp_review_new'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('cascade-impl-bot-new') + .mockResolvedValueOnce('cascade-review-bot-new'); + + const result = await resolvePersonaIdentities('project1'); + // 4 total calls (2 original + 2 re-fetch) + expect(getIntegrationCredential).toHaveBeenCalledTimes(4); + expect(result.implementer).toBe('cascade-impl-bot-new'); + } finally { + vi.useRealTimers(); + } + }); + + it('isolates cache per project ID', async () => { + // Set up project1 in cache + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl_1') + .mockResolvedValueOnce('ghp_review_1'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('bot-impl-1') + .mockResolvedValueOnce('bot-review-1'); + + const result1a = await resolvePersonaIdentities('project1'); + + // Populate project2 + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl_2') + .mockResolvedValueOnce('ghp_review_2'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('bot-impl-2') + .mockResolvedValueOnce('bot-review-2'); + + const result2 = await resolvePersonaIdentities('project2'); + + // project1 second call — served from cache (no additional mock calls needed) + const result1b = await resolvePersonaIdentities('project1'); + + // project1 identity is preserved across project2 population + expect(result1a.implementer).toBe('bot-impl-1'); + expect(result1b.implementer).toBe('bot-impl-1'); + expect(result1b).toStrictEqual(result1a); + // project2 has its own separate cached identity + expect(result2.implementer).toBe('bot-impl-2'); + // Only 4 credential lookups total (project1 once + project2 once) + expect(getIntegrationCredential).toHaveBeenCalledTimes(4); + }); + + it('re-fetches after cache is manually reset', async () => { + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl') + .mockResolvedValueOnce('ghp_review'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('cascade-impl-bot') + .mockResolvedValueOnce('cascade-review-bot'); + + await resolvePersonaIdentities('project1'); + expect(getIntegrationCredential).toHaveBeenCalledTimes(2); + + // Reset the cache + _resetPersonaIdentityCache(); + + vi.mocked(getIntegrationCredential) + .mockResolvedValueOnce('ghp_impl') + .mockResolvedValueOnce('ghp_review'); + vi.mocked(getGitHubUserForToken) + .mockResolvedValueOnce('cascade-impl-bot') + .mockResolvedValueOnce('cascade-review-bot'); + + await resolvePersonaIdentities('project1'); + // 4 total calls (2 original + 2 after reset) + expect(getIntegrationCredential).toHaveBeenCalledTimes(4); + }); }); // ======================================================================== diff --git a/tests/unit/router/adapters/github.test.ts b/tests/unit/router/adapters/github.test.ts index 549fa7ab..feecb059 100644 --- a/tests/unit/router/adapters/github.test.ts +++ b/tests/unit/router/adapters/github.test.ts @@ -477,4 +477,85 @@ describe('GitHubRouterAdapter', () => { expect(addEyesReactionToPR).not.toHaveBeenCalled(); }); }); + + describe('per-request caching', () => { + it('calls findProjectByRepo only once across isSelfAuthored and sendReaction', async () => { + vi.mocked(findProjectByRepo).mockResolvedValue({ id: 'p1' } as never); + vi.mocked(resolvePersonaIdentities).mockResolvedValue({ + implementer: 'impl-bot', + reviewer: 'review-bot', + } as never); + vi.mocked(isCascadeBot).mockReturnValue(false); + vi.mocked(sendAcknowledgeReaction).mockResolvedValue(undefined); + + const commentEvent = { + projectIdentifier: 'owner/repo', + eventType: 'issue_comment', + isCommentEvent: true, + // @ts-expect-error extended field + repoFullName: 'owner/repo', + }; + + // Both calls share the same adapter instance + await adapter.isSelfAuthored(commentEvent, { comment: { user: { login: 'human' } } }); + adapter.sendReaction(commentEvent, { comment: { body: 'hello' } }); + + // Give the async fire-and-forget in sendReaction time to execute + await vi.waitFor(() => expect(sendAcknowledgeReaction).toHaveBeenCalled()); + + // findProjectByRepo should have been called only once (cached for the second call) + expect(findProjectByRepo).toHaveBeenCalledTimes(1); + // resolvePersonaIdentities should have been called only once + expect(resolvePersonaIdentities).toHaveBeenCalledTimes(1); + }); + + it('calls resolvePersonaIdentities only once across isSelfAuthored and dispatchWithCredentials', async () => { + vi.mocked(findProjectByRepo).mockResolvedValue({ id: 'p1' } as never); + vi.mocked(resolvePersonaIdentities).mockResolvedValue({ + implementer: 'impl-bot', + reviewer: 'review-bot', + } as never); + vi.mocked(isCascadeBot).mockReturnValue(false); + + const commentEvent = { + projectIdentifier: 'owner/repo', + eventType: 'issue_comment', + isCommentEvent: true, + // @ts-expect-error extended field + repoFullName: 'owner/repo', + }; + + await adapter.isSelfAuthored(commentEvent, { comment: { user: { login: 'human' } } }); + await adapter.dispatchWithCredentials(commentEvent, {}, mockProject, mockTriggerRegistry); + + // resolvePersonaIdentities called only once (cached by dispatchWithCredentials for the same projectId) + expect(resolvePersonaIdentities).toHaveBeenCalledTimes(1); + }); + + it('does not call loadProjectConfig again in postAck when dispatchWithCredentials ran first', async () => { + vi.mocked(resolvePersonaIdentities).mockResolvedValue({} as never); + vi.mocked(resolveGitHubTokenForAckByAgent).mockResolvedValue({ + token: 'ghp_test', + project: { id: 'p1' }, + } as never); + vi.mocked(extractPRNumber).mockReturnValue(42); + vi.mocked(postGitHubAck).mockResolvedValue(999); + + const event = { + projectIdentifier: 'owner/repo', + eventType: 'pull_request', + workItemId: '42', + isCommentEvent: false, + // @ts-expect-error extended field + repoFullName: 'owner/repo', + }; + + await adapter.dispatchWithCredentials(event, {}, mockProject, mockTriggerRegistry); + await adapter.postAck(event, {}, mockProject, 'review'); + + // loadProjectConfig should have been called once (by dispatchWithCredentials) + // postAck reuses cachedFullProject instead of calling loadProjectConfig again + expect(loadProjectConfig).toHaveBeenCalledTimes(1); + }); + }); }); diff --git a/tests/unit/router/github-token-resolver.test.ts b/tests/unit/router/github-token-resolver.test.ts index df433069..6c9260b8 100644 --- a/tests/unit/router/github-token-resolver.test.ts +++ b/tests/unit/router/github-token-resolver.test.ts @@ -136,4 +136,52 @@ describe('resolveGitHubTokenForAckByAgent', () => { expect(result).toBeNull(); }); + + it('skips findProjectByRepo when pre-resolved project is provided', async () => { + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'test-reviewer-token'; + throw new Error(`Credential '${category}/${role}' not found`); + }); + + const preResolvedProject = { + id: 'test', + name: 'Test', + repo: 'owner/repo', + baseBranch: 'main', + branchPrefix: 'feature/', + } as never; + + const result = await resolveGitHubTokenForAckByAgent( + 'owner/repo', + 'review', + preResolvedProject, + ); + + expect(result).not.toBeNull(); + expect(result?.token).toBe('test-reviewer-token'); + expect(result?.project.id).toBe('test'); + // findProjectByRepo should NOT have been called since we passed the project directly + expect(mockFindProjectByRepo).not.toHaveBeenCalled(); + }); + + it('uses pre-resolved project for implementer token when provided', async () => { + const preResolvedProject = { + id: 'test', + name: 'Test', + repo: 'owner/repo', + baseBranch: 'main', + branchPrefix: 'feature/', + } as never; + + const result = await resolveGitHubTokenForAckByAgent( + 'owner/repo', + 'implementation', + preResolvedProject, + ); + + expect(result).not.toBeNull(); + expect(result?.token).toBe('test-github-token'); + expect(mockFindProjectByRepo).not.toHaveBeenCalled(); + expect(mockGetProjectGitHubToken).toHaveBeenCalledWith(preResolvedProject); + }); });