diff --git a/src/agents/utils/tracking.ts b/src/agents/utils/tracking.ts index 21709695..c8e2ced1 100644 --- a/src/agents/utils/tracking.ts +++ b/src/agents/utils/tracking.ts @@ -72,6 +72,12 @@ export interface TrackingContext { loopDetection: LoopDetectionState; /** Agent type for role-aware loop messages */ agentType?: string; + /** + * Loop advice profile to use for role-aware messages. + * When set, takes precedence over the agentType-based lookup. + * Derived from `profile.finishHooks.requiresReview`. + */ + loopAdviceProfile?: 'review' | 'default'; } /** @@ -92,12 +98,16 @@ export function createLoopDetectionState(): LoopDetectionState { /** * Create a new tracking context with zero metrics. */ -export function createTrackingContext(agentType?: string): TrackingContext { +export function createTrackingContext( + agentType?: string, + loopAdviceProfile?: 'review' | 'default', +): TrackingContext { return { metrics: { llmIterations: 0, gadgetCalls: 0 }, syntheticInvocationIds: new Set(), loopDetection: createLoopDetectionState(), agentType, + loopAdviceProfile, }; } @@ -215,8 +225,13 @@ const LOOP_ADVICE = { }, } as const; -function getAdvice(agentType?: string): (typeof LOOP_ADVICE)[keyof typeof LOOP_ADVICE] { - return agentType === 'review' ? LOOP_ADVICE.review : LOOP_ADVICE.default; +function getAdvice( + agentType?: string, + loopAdviceProfile?: 'review' | 'default', +): (typeof LOOP_ADVICE)[keyof typeof LOOP_ADVICE] { + const isReview = + loopAdviceProfile !== undefined ? loopAdviceProfile === 'review' : agentType === 'review'; + return isReview ? LOOP_ADVICE.review : LOOP_ADVICE.default; } /** @@ -236,13 +251,14 @@ function generateLoopWarning( repeatCount: number, repeatedPattern: string, agentType?: string, + loopAdviceProfile?: 'review' | 'default', ): string { const urgency = repeatCount >= 3 ? '🚨' : '⚠️'; return `[System] ${urgency} LOOP DETECTED (×${repeatCount}) Pattern: ${repeatedPattern} -${getAdvice(agentType).exact}`; +${getAdvice(agentType, loopAdviceProfile).exact}`; } /** @@ -252,9 +268,11 @@ function generateNameOnlyLoopAction( repeatCount: number, pattern: string, agentType?: string, + loopAdviceProfile?: 'review' | 'default', ): LoopAction | null { - const isReview = agentType === 'review'; - const advice = getAdvice(agentType); + const isReview = + loopAdviceProfile !== undefined ? loopAdviceProfile === 'review' : agentType === 'review'; + const advice = getAdvice(agentType, loopAdviceProfile); if (repeatCount >= LOOP_THRESHOLDS.HARD_STOP) { return { @@ -324,6 +342,7 @@ export function checkForLoopAndAdvance(context: TrackingContext): boolean { state.repeatCount, state.repeatedPattern, context.agentType, + context.loopAdviceProfile, ); } else { state.repeatCount = 1; @@ -346,6 +365,7 @@ export function checkForLoopAndAdvance(context: TrackingContext): boolean { state.nameOnlyRepeatCount, formatCallsForDisplay(state.currentIterationCalls), context.agentType, + context.loopAdviceProfile, ); } else { state.nameOnlyRepeatCount = 1; diff --git a/src/backends/llmist/index.ts b/src/backends/llmist/index.ts index 8db7ab48..e078c42a 100644 --- a/src/backends/llmist/index.ts +++ b/src/backends/llmist/index.ts @@ -65,7 +65,10 @@ export class LlmistEngine implements AgentEngine { // Create per-execution llmist logger and tracking state const llmistLogger = createLogger({ minLevel: getLogLevel() }); - const trackingContext = createTrackingContext(agentType); + const trackingContext = createTrackingContext( + agentType, + profile.finishHooks.requiresReview ? 'review' : 'default', + ); const llmCallAccumulator: AccumulatedLlmCall[] = []; // Create a LLM call logger for raw request/response file logging. diff --git a/src/backends/sidecarManager.ts b/src/backends/sidecarManager.ts index 3fd21798..0541369f 100644 --- a/src/backends/sidecarManager.ts +++ b/src/backends/sidecarManager.ts @@ -23,7 +23,7 @@ import type { AgentEngineResult } from './types.js'; */ export function createCompletionArtifacts( profile: Awaited>, - agentType: string, + _agentType: string, needsNativeToolRuntime: boolean, input: AgentInput, projectSecrets: Record, @@ -33,10 +33,9 @@ export function createCompletionArtifacts( reviewSidecarPath: string | undefined; pmWriteSidecarPath: string | undefined; } { - const reviewSidecarPath = - agentType === 'review' - ? join(tmpdir(), `cascade-review-sidecar-${process.pid}-${Date.now()}.json`) - : undefined; + const reviewSidecarPath = profile.finishHooks.requiresReview + ? join(tmpdir(), `cascade-review-sidecar-${process.pid}-${Date.now()}.json`) + : undefined; if (reviewSidecarPath) { projectSecrets[REVIEW_SIDECAR_ENV_VAR] = reviewSidecarPath; } diff --git a/src/github/personas.ts b/src/github/personas.ts index 588acbf3..45496f8e 100644 --- a/src/github/personas.ts +++ b/src/github/personas.ts @@ -17,6 +17,20 @@ export interface PersonaIdentities { // Agent → Persona Mapping // ============================================================================ +/** + * Maps agent types to their GitHub personas. + * + * This is the canonical registration point for agent persona assignments. + * - `'implementer'` — uses the implementer GitHub token for all SCM operations + * - `'reviewer'` — uses the reviewer GitHub token, appropriate for agents + * that submit PR reviews (e.g. the built-in `review` agent) + * + * To add a custom agent with reviewer behaviour, add an entry here: + * ```ts + * 'my-custom-reviewer': 'reviewer', + * ``` + * Any agent type not listed here defaults to `'implementer'`. + */ const AGENT_PERSONA_MAP: Record = { splitting: 'implementer', planning: 'implementer', diff --git a/src/router/github-token-resolver.ts b/src/router/github-token-resolver.ts index 4866919a..6dc27f97 100644 --- a/src/router/github-token-resolver.ts +++ b/src/router/github-token-resolver.ts @@ -9,6 +9,7 @@ import { getProjectGitHubToken } from '../config/projects.js'; import { findProjectByRepo, getIntegrationCredential } from '../config/provider.js'; +import { getPersonaForAgentType } from '../github/personas.js'; import type { ProjectConfig } from '../types/index.js'; import { logger } from '../utils/logging.js'; @@ -57,7 +58,8 @@ export async function resolveGitHubTokenForAckByAgent( if (!resolvedProject) return null; try { - if (agentType === 'review') { + const persona = getPersonaForAgentType(agentType); + if (persona === 'reviewer') { const token = await getIntegrationCredential(resolvedProject.id, 'scm', 'reviewer_token'); return { token, project: resolvedProject }; } diff --git a/tests/unit/agents/utils/tracking.test.ts b/tests/unit/agents/utils/tracking.test.ts index aebb1a25..e9e1c5d0 100644 --- a/tests/unit/agents/utils/tracking.test.ts +++ b/tests/unit/agents/utils/tracking.test.ts @@ -519,4 +519,51 @@ describe('loop detection', () => { expect(action?.message).toContain('delete the failing test'); }); }); + + describe('loopAdviceProfile overrides agentType', () => { + it('uses review loop advice when loopAdviceProfile is "review" and agentType is not "review"', () => { + const ctx = createTrackingContext('implementation', 'review'); + + // Create exact-match loop to trigger warning + recordGadgetCallForLoop(ctx, 'ReadFile', { filePath: '/foo.ts' }); + checkForLoopAndAdvance(ctx); + recordGadgetCallForLoop(ctx, 'ReadFile', { filePath: '/foo.ts' }); + checkForLoopAndAdvance(ctx); + + const warning = consumeLoopWarning(ctx); + expect(warning).toContain('CreatePRReview'); + expect(warning).not.toContain('COMPLETELY DIFFERENT APPROACH'); + }); + + it('uses default loop advice when loopAdviceProfile is "default" and agentType is "review"', () => { + const ctx = createTrackingContext('review', 'default'); + + // Create exact-match loop to trigger warning + recordGadgetCallForLoop(ctx, 'ReadFile', { filePath: '/foo.ts' }); + checkForLoopAndAdvance(ctx); + recordGadgetCallForLoop(ctx, 'ReadFile', { filePath: '/foo.ts' }); + checkForLoopAndAdvance(ctx); + + const warning = consumeLoopWarning(ctx); + expect(warning).toContain('COMPLETELY DIFFERENT APPROACH'); + expect(warning).not.toContain('CreatePRReview'); + }); + + it('uses review name-only loop action when loopAdviceProfile is "review" and agentType is not "review"', () => { + const ctx = createTrackingContext('implementation', 'review'); + + for (let i = 0; i < LOOP_THRESHOLDS.WARNING; i++) { + recordGadgetCallForLoop(ctx, 'FileSearchAndReplace', { + filePath: '/foo.ts', + search: `v${i}`, + }); + checkForLoopAndAdvance(ctx); + } + + const action = consumeLoopAction(ctx); + expect(action).not.toBeNull(); + expect(action?.message).toContain('CreatePRReview'); + expect(action?.message).not.toContain('delete the failing test'); + }); + }); }); diff --git a/tests/unit/backends/adapter.test.ts b/tests/unit/backends/adapter.test.ts index cef03809..420bb3fc 100644 --- a/tests/unit/backends/adapter.test.ts +++ b/tests/unit/backends/adapter.test.ts @@ -854,6 +854,9 @@ describe('executeWithEngine', () => { it('calls recordReviewSubmission when sidecar exists for review agent', async () => { setupMocks(); + mockGetAgentProfile.mockReturnValue( + makeMockProfile({ finishHooks: { requiresReview: true } }), + ); const engine = makeMockBackend(); writeSidecarAtInjectedPath(engine, { reviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-99', @@ -873,6 +876,9 @@ describe('executeWithEngine', () => { it('injects CASCADE_REVIEW_SIDECAR_PATH into projectSecrets for review agent', async () => { setupMocks(); + mockGetAgentProfile.mockReturnValue( + makeMockProfile({ finishHooks: { requiresReview: true } }), + ); const engine = makeMockBackend(); const input = makeInput(); @@ -951,6 +957,9 @@ describe('executeWithEngine', () => { it('clears initialCommentId when sidecar has ackCommentDeleted: true', async () => { setupMocks(); + mockGetAgentProfile.mockReturnValue( + makeMockProfile({ finishHooks: { requiresReview: true } }), + ); const engine = makeMockBackend(); writeSidecarAtInjectedPath(engine, { reviewUrl: 'https://github.com/o/r/pull/1#pullrequestreview-42', diff --git a/tests/unit/backends/llmist.test.ts b/tests/unit/backends/llmist.test.ts index 05153693..1e2ecc08 100644 --- a/tests/unit/backends/llmist.test.ts +++ b/tests/unit/backends/llmist.test.ts @@ -22,6 +22,7 @@ vi.mock('../../../src/agents/definitions/index.js', () => ({ vi.mock('../../../src/agents/definitions/profiles.js', () => ({ getAgentProfile: vi.fn(() => ({ getLlmistGadgets: vi.fn(() => []), + finishHooks: {}, })), })); @@ -363,6 +364,7 @@ describe('LlmistEngine.execute', () => { const mockGetLlmistGadgets = vi.fn().mockReturnValue([]); mockGetAgentProfile.mockReturnValue({ getLlmistGadgets: mockGetLlmistGadgets, + finishHooks: {}, } as ReturnType); const engine = new LlmistEngine(); diff --git a/tests/unit/backends/sidecarManager.test.ts b/tests/unit/backends/sidecarManager.test.ts index 8a3bfcd9..2bd4ac5f 100644 --- a/tests/unit/backends/sidecarManager.test.ts +++ b/tests/unit/backends/sidecarManager.test.ts @@ -59,8 +59,8 @@ function makeSidecarPath(name: string): string { } describe('createCompletionArtifacts', () => { - it('creates a review sidecar path for review agent type', () => { - const profile = makeProfile(); + it('creates a review sidecar path when profile.finishHooks.requiresReview is true', () => { + const profile = makeProfile({ finishHooks: { requiresReview: true } }); const projectSecrets: Record = {}; const result = createCompletionArtifacts( @@ -91,6 +91,38 @@ describe('createCompletionArtifacts', () => { expect(projectSecrets.CASCADE_REVIEW_SIDECAR_PATH).toBeUndefined(); }); + it('creates review sidecar for custom agent with requiresReview: true', () => { + const profile = makeProfile({ finishHooks: { requiresReview: true } }); + const projectSecrets: Record = {}; + + const result = createCompletionArtifacts( + profile, + 'custom-reviewer', + false, + {} as AgentInput, + projectSecrets, + ); + + expect(result.reviewSidecarPath).toMatch(/cascade-review-sidecar-\d+-\d+\.json$/); + expect(projectSecrets.CASCADE_REVIEW_SIDECAR_PATH).toBe(result.reviewSidecarPath); + }); + + it('does not create review sidecar when requiresReview is not set', () => { + const profile = makeProfile({ finishHooks: {} }); + const projectSecrets: Record = {}; + + const result = createCompletionArtifacts( + profile, + 'implementation', + false, + {} as AgentInput, + projectSecrets, + ); + + expect(result.reviewSidecarPath).toBeUndefined(); + expect(projectSecrets.CASCADE_REVIEW_SIDECAR_PATH).toBeUndefined(); + }); + it('creates a PR sidecar path when requiresPR and needsNativeToolRuntime', () => { const profile = makeProfile({ finishHooks: { requiresPR: true } }); const projectSecrets: Record = {}; diff --git a/tests/unit/router/github-token-resolver.test.ts b/tests/unit/router/github-token-resolver.test.ts index 6c9260b8..29e39aa2 100644 --- a/tests/unit/router/github-token-resolver.test.ts +++ b/tests/unit/router/github-token-resolver.test.ts @@ -11,6 +11,13 @@ vi.mock('../../../src/config/projects.js', () => ({ getProjectGitHubToken: vi.fn(), })); +// Mock getPersonaForAgentType for persona-based token selection tests +vi.mock('../../../src/github/personas.js', () => ({ + getPersonaForAgentType: vi.fn((agentType: string) => + agentType === 'review' ? 'reviewer' : 'implementer', + ), +})); + // Mock config cache (imported transitively) vi.mock('../../../src/config/configCache.js', () => ({ configCache: { @@ -36,11 +43,14 @@ vi.mock('../../../src/utils/logging.js', () => ({ import { getProjectGitHubToken } from '../../../src/config/projects.js'; import { findProjectByRepo, getIntegrationCredential } from '../../../src/config/provider.js'; +import { getPersonaForAgentType } from '../../../src/github/personas.js'; import { resolveGitHubTokenForAck, resolveGitHubTokenForAckByAgent, } from '../../../src/router/github-token-resolver.js'; +const mockGetPersonaForAgentType = vi.mocked(getPersonaForAgentType); + const mockGetIntegrationCredential = vi.mocked(getIntegrationCredential); const mockGetProjectGitHubToken = vi.mocked(getProjectGitHubToken); const mockFindProjectByRepo = vi.mocked(findProjectByRepo); @@ -184,4 +194,29 @@ describe('resolveGitHubTokenForAckByAgent', () => { expect(mockFindProjectByRepo).not.toHaveBeenCalled(); expect(mockGetProjectGitHubToken).toHaveBeenCalledWith(preResolvedProject); }); + + it('delegates to getPersonaForAgentType for reviewer token selection', async () => { + mockGetPersonaForAgentType.mockReturnValueOnce('reviewer'); + mockGetIntegrationCredential.mockImplementation(async (_projectId, category, role) => { + if (category === 'scm' && role === 'reviewer_token') return 'custom-reviewer-token'; + throw new Error(`Credential '${category}/${role}' not found`); + }); + + const result = await resolveGitHubTokenForAckByAgent('owner/repo', 'custom-review-agent'); + + expect(result).not.toBeNull(); + expect(result?.token).toBe('custom-reviewer-token'); + expect(mockGetPersonaForAgentType).toHaveBeenCalledWith('custom-review-agent'); + }); + + it('delegates to getPersonaForAgentType for implementer token selection', async () => { + mockGetPersonaForAgentType.mockReturnValueOnce('implementer'); + + const result = await resolveGitHubTokenForAckByAgent('owner/repo', 'custom-impl-agent'); + + expect(result).not.toBeNull(); + expect(result?.token).toBe('test-github-token'); + expect(mockGetPersonaForAgentType).toHaveBeenCalledWith('custom-impl-agent'); + expect(mockGetProjectGitHubToken).toHaveBeenCalled(); + }); });