From d871f981d92fbdf1a24fa2675df6f94ba4b62ca0 Mon Sep 17 00:00:00 2001 From: Cascade Bot Date: Mon, 2 Mar 2026 15:01:31 +0000 Subject: [PATCH] refactor(hooks): replace flat backend flags with hooks.scm category-scoped infrastructure --- src/agents/definitions/debug.yaml | 1 - src/agents/definitions/email-joke.yaml | 1 - src/agents/definitions/implementation.yaml | 6 +- src/agents/definitions/planning.yaml | 1 - src/agents/definitions/profiles.ts | 42 +++++++++- src/agents/definitions/respond-to-ci.yaml | 7 +- .../respond-to-planning-comment.yaml | 1 - .../definitions/respond-to-pr-comment.yaml | 7 +- src/agents/definitions/respond-to-review.yaml | 7 +- src/agents/definitions/review.yaml | 5 +- src/agents/definitions/schema.ts | 48 ++++++++++- src/agents/definitions/splitting.yaml | 1 - src/agents/shared/builderFactory.ts | 12 ++- src/backends/llmist/index.ts | 6 ++ src/cli/session/finish.ts | 1 + src/gadgets/Finish.ts | 1 + src/gadgets/session/core/finish.ts | 20 +++-- src/gadgets/sessionState.ts | 16 ++++ tests/unit/agents/definitions/loader.test.ts | 12 +-- .../unit/agents/shared/builderFactory.test.ts | 2 + tests/unit/gadgets/finish.test.ts | 26 +++--- .../unit/gadgets/session/core/finish.test.ts | 71 +++++++++++------ .../settings/agent-definition-editor.tsx | 79 ++++++++++++++----- 23 files changed, 286 insertions(+), 87 deletions(-) diff --git a/src/agents/definitions/debug.yaml b/src/agents/definitions/debug.yaml index 83bab398..e4af00e9 100644 --- a/src/agents/definitions/debug.yaml +++ b/src/agents/definitions/debug.yaml @@ -31,7 +31,6 @@ prompts: Analyze and process the work item with ID: <%= it.cardId %>. The work item data has been pre-loaded. backend: - enableStopHooks: true needsGitHubToken: false diff --git a/src/agents/definitions/email-joke.yaml b/src/agents/definitions/email-joke.yaml index c1f68f26..9efde644 100644 --- a/src/agents/definitions/email-joke.yaml +++ b/src/agents/definitions/email-joke.yaml @@ -55,7 +55,6 @@ prompts: <% } %> backend: - enableStopHooks: false needsGitHubToken: false diff --git a/src/agents/definitions/implementation.yaml b/src/agents/definitions/implementation.yaml index e2029d37..fa53a5ff 100644 --- a/src/agents/definitions/implementation.yaml +++ b/src/agents/definitions/implementation.yaml @@ -68,9 +68,11 @@ prompts: Analyze and process the work item with ID: <%= it.cardId %>. The work item data has been pre-loaded. backend: - enableStopHooks: true needsGitHubToken: true - requiresPR: true + hooks: + scm: + enableStopHooks: true + requiresPR: true postConfigure: sequentialGadgetExecution diff --git a/src/agents/definitions/planning.yaml b/src/agents/definitions/planning.yaml index 3bbd7cbc..2acb0f5a 100644 --- a/src/agents/definitions/planning.yaml +++ b/src/agents/definitions/planning.yaml @@ -70,7 +70,6 @@ prompts: Analyze and process the work item with ID: <%= it.cardId %>. The work item data has been pre-loaded. backend: - enableStopHooks: false needsGitHubToken: false diff --git a/src/agents/definitions/profiles.ts b/src/agents/definitions/profiles.ts index e5475bff..0d16ce95 100644 --- a/src/agents/definitions/profiles.ts +++ b/src/agents/definitions/profiles.ts @@ -25,6 +25,7 @@ import type { AgentCapabilities, AgentDefinition, ContextStepName, + ScmHooks, SupportedTrigger, } from './schema.js'; import { CONTEXT_STEP_REGISTRY, PRE_EXECUTE_REGISTRY } from './strategies.js'; @@ -49,6 +50,10 @@ export interface AgentProfile { blockGitPush?: boolean; /** Whether the agent must create a PR for success (e.g., implementation) */ requiresPR?: boolean; + /** Whether the agent must submit a review before finishing (e.g., review) */ + requiresReview?: boolean; + /** Whether the agent must have pushed changes before finishing */ + requiresPushedChanges?: boolean; /** Fetch context injections for this agent type */ fetchContext(params: FetchContextParams): Promise; /** Build the task prompt for this agent type */ @@ -72,6 +77,32 @@ export interface AgentProfile { // Helpers // ============================================================================ +/** + * Resolve the effective SCM hooks configuration from an agent definition. + * + * Merges legacy flat flags with the new `hooks.scm` format. + * The new `hooks.scm` format wins when both are present. + * + * Legacy flat flags (deprecated): + * - `backend.enableStopHooks` → `hooks.scm.enableStopHooks` + * - `backend.blockGitPush` → `hooks.scm.blockGitPush` + * - `backend.requiresPR` → `hooks.scm.requiresPR` + * + * New fields only available via `hooks.scm`: + * - `hooks.scm.requiresReview` + * - `hooks.scm.requiresPushedChanges` + */ +export function resolveScmHooks(backend: AgentDefinition['backend']): ScmHooks { + const legacy: ScmHooks = { + ...(backend.enableStopHooks !== undefined && { enableStopHooks: backend.enableStopHooks }), + ...(backend.blockGitPush !== undefined && { blockGitPush: backend.blockGitPush }), + ...(backend.requiresPR !== undefined && { requiresPR: backend.requiresPR }), + }; + const newHooks = backend.hooks?.scm ?? {}; + // New hooks.scm wins over legacy flat flags + return { ...legacy, ...newHooks }; +} + function resolveRegistry(registry: Record, key: string, label: string): T { const value = registry[key]; if (!value) throw new Error(`${label} '${key}' not found in registry`); @@ -146,6 +177,9 @@ function buildProfileFromDefinition(def: AgentDefinition, agentType: string): Ag throw new Error(`Agent '${agentType}' has invalid taskPrompt: ${validationResult.error}`); } + // Resolve SCM hooks (merges legacy flat flags with new hooks.scm format) + const scmHooks = resolveScmHooks(def.backend); + const profile: AgentProfile = { filterTools: (allTools: ToolManifest[]) => { // Filter tools by the gadget names derived from capabilities @@ -153,10 +187,12 @@ function buildProfileFromDefinition(def: AgentDefinition, agentType: string): Ag return allTools.filter((t) => nameSet.has(t.name)); }, sdkTools, - enableStopHooks: def.backend.enableStopHooks, + enableStopHooks: scmHooks.enableStopHooks ?? false, needsGitHubToken: def.backend.needsGitHubToken, - ...(def.backend.blockGitPush !== undefined && { blockGitPush: def.backend.blockGitPush }), - ...(def.backend.requiresPR && { requiresPR: true }), + ...(scmHooks.blockGitPush !== undefined && { blockGitPush: scmHooks.blockGitPush }), + ...(scmHooks.requiresPR && { requiresPR: true }), + ...(scmHooks.requiresReview && { requiresReview: true }), + ...(scmHooks.requiresPushedChanges && { requiresPushedChanges: true }), fetchContext: async (params) => { // Resolve context pipeline from the trigger (empty array if no trigger or trigger has no pipeline) const contextPipeline = resolveContextPipeline(triggers, params.input.triggerType); diff --git a/src/agents/definitions/respond-to-ci.yaml b/src/agents/definitions/respond-to-ci.yaml index 0e06a4d3..aa95bdaa 100644 --- a/src/agents/definitions/respond-to-ci.yaml +++ b/src/agents/definitions/respond-to-ci.yaml @@ -41,9 +41,12 @@ prompts: CI checks have failed. Analyze the failures and fix them. backend: - enableStopHooks: true needsGitHubToken: true - blockGitPush: false + hooks: + scm: + enableStopHooks: true + blockGitPush: false + requiresPushedChanges: true preExecute: postInitialPRComment diff --git a/src/agents/definitions/respond-to-planning-comment.yaml b/src/agents/definitions/respond-to-planning-comment.yaml index b8cf7b27..8a1e9bd1 100644 --- a/src/agents/definitions/respond-to-planning-comment.yaml +++ b/src/agents/definitions/respond-to-planning-comment.yaml @@ -44,7 +44,6 @@ prompts: Read the user's comment carefully and classify it: if they ask a question or request clarification, reply with a thorough answer via PostComment (do not modify the plan). If they request plan changes, make surgical, targeted updates. If the comment contains both a question and a change request, do both. Default to plan updates when intent is ambiguous. backend: - enableStopHooks: false needsGitHubToken: false diff --git a/src/agents/definitions/respond-to-pr-comment.yaml b/src/agents/definitions/respond-to-pr-comment.yaml index 6a1b1ec8..ec410528 100644 --- a/src/agents/definitions/respond-to-pr-comment.yaml +++ b/src/agents/definitions/respond-to-pr-comment.yaml @@ -52,9 +52,12 @@ prompts: Read the comment carefully and respond accordingly. If they ask for code changes, make the changes, commit, and push. If they ask a question, reply with a PR comment. Default to surgical, targeted changes unless they clearly ask for something broader. backend: - enableStopHooks: true needsGitHubToken: true - blockGitPush: false + hooks: + scm: + enableStopHooks: true + blockGitPush: false + requiresPushedChanges: true hint: Complete the current task efficiently before moving to the next. diff --git a/src/agents/definitions/respond-to-review.yaml b/src/agents/definitions/respond-to-review.yaml index 253b004c..f1d31a2c 100644 --- a/src/agents/definitions/respond-to-review.yaml +++ b/src/agents/definitions/respond-to-review.yaml @@ -50,9 +50,12 @@ prompts: Carefully read each review comment and make the requested changes. Commit and push your changes when done. Use the ReplyToReviewComment tool to respond to individual review comments as you address them. Focus on surgical, targeted fixes unless the reviewer clearly asks for broader changes. backend: - enableStopHooks: true needsGitHubToken: true - blockGitPush: false + hooks: + scm: + enableStopHooks: true + blockGitPush: false + requiresPushedChanges: true hint: Address the current review comment fully before moving to the next. Batch related file edits together. diff --git a/src/agents/definitions/review.yaml b/src/agents/definitions/review.yaml index 70dd7f5c..518ba469 100644 --- a/src/agents/definitions/review.yaml +++ b/src/agents/definitions/review.yaml @@ -59,8 +59,11 @@ prompts: Examine the code changes carefully and submit your review using CreatePRReview. backend: - enableStopHooks: false needsGitHubToken: true + hooks: + scm: + enableStopHooks: false + requiresReview: true preExecute: postInitialPRComment diff --git a/src/agents/definitions/schema.ts b/src/agents/definitions/schema.ts index df2fd7db..4c49199b 100644 --- a/src/agents/definitions/schema.ts +++ b/src/agents/definitions/schema.ts @@ -206,11 +206,51 @@ const StrategiesSchema = z.object({ gadgetOptions: GadgetOptionsSchema, }); +/** + * SCM-specific hook configuration. + * Controls stop-hook behavior and push/PR requirements for SCM-integrated agents. + */ +export const ScmHooksSchema = z.object({ + /** Whether to enable stop hooks that check for uncommitted/unpushed changes */ + enableStopHooks: z.boolean().optional(), + /** Whether to block git push in hooks (set false for agents working on existing PR branches) */ + blockGitPush: z.boolean().optional(), + /** Whether the agent must create a PR before finishing */ + requiresPR: z.boolean().optional(), + /** Whether the agent must submit a review before finishing */ + requiresReview: z.boolean().optional(), + /** Whether the agent must have pushed changes before finishing */ + requiresPushedChanges: z.boolean().optional(), +}); + +/** + * Category-scoped hook configuration. + * Extensible for future categories (e.g., hooks.email, hooks.pm). + */ +export const HooksSchema = z.object({ + /** SCM (source control) hook configuration */ + scm: ScmHooksSchema.optional(), +}); + const BackendSchema = z.object({ - enableStopHooks: z.boolean(), + /** + * @deprecated Use hooks.scm.enableStopHooks instead. + * Kept for backward compatibility — new format wins when both are present. + */ + enableStopHooks: z.boolean().optional(), needsGitHubToken: z.boolean(), + /** + * @deprecated Use hooks.scm.blockGitPush instead. + * Kept for backward compatibility — new format wins when both are present. + */ blockGitPush: z.boolean().optional(), + /** + * @deprecated Use hooks.scm.requiresPR instead. + * Kept for backward compatibility — new format wins when both are present. + */ requiresPR: z.boolean().optional(), + /** Category-scoped hook configuration */ + hooks: HooksSchema.optional(), preExecute: z.enum(['postInitialPRComment']).optional(), postConfigure: z.enum(['sequentialGadgetExecution']).optional(), }); @@ -302,3 +342,9 @@ export type IntegrationRequirements = z.infer; + +/** SCM hook configuration */ +export type ScmHooks = z.infer; + +/** Category-scoped hook configuration */ +export type Hooks = z.infer; diff --git a/src/agents/definitions/splitting.yaml b/src/agents/definitions/splitting.yaml index 6b3c63dd..659f30b4 100644 --- a/src/agents/definitions/splitting.yaml +++ b/src/agents/definitions/splitting.yaml @@ -66,7 +66,6 @@ prompts: Analyze and process the work item with ID: <%= it.cardId %>. The work item data has been pre-loaded. backend: - enableStopHooks: false needsGitHubToken: false diff --git a/src/agents/shared/builderFactory.ts b/src/agents/shared/builderFactory.ts index 6e8a8a05..0229b466 100644 --- a/src/agents/shared/builderFactory.ts +++ b/src/agents/shared/builderFactory.ts @@ -9,7 +9,7 @@ import { getCompactionConfig } from '../../config/compactionConfig.js'; import { getIterationTrailingMessage } from '../../config/hintConfig.js'; import { getRateLimitForModel } from '../../config/rateLimits.js'; import { getRetryConfig } from '../../config/retryConfig.js'; -import { initSessionState } from '../../gadgets/sessionState.js'; +import { type SessionHooks, initSessionState } from '../../gadgets/sessionState.js'; import type { LLMCallLogger } from '../../utils/llmLogging.js'; import { resolveSquintDbPath } from '../../utils/squintDb.js'; import type { IProgressMonitor } from '../contracts/index.js'; @@ -48,6 +48,8 @@ export interface CreateBuilderOptions { projectId?: string; /** Work item (card) ID for PR ↔ work item linking. Passed to session state. */ cardId?: string; + /** Resolved SCM hook flags for finish validation (requiresPR, requiresReview, etc.) */ + hooks?: SessionHooks; } const MAX_GADGETS_PER_RESPONSE = 25; @@ -76,7 +78,13 @@ export async function createConfiguredBuilder(options: CreateBuilderOptions): Pr // Initialize session state for gadgets (e.g., Finish checks PR requirement for implementation) if (!skipSessionState) { - initSessionState(agentType, options.baseBranch, options.projectId, options.cardId); + initSessionState( + agentType, + options.baseBranch, + options.projectId, + options.cardId, + options.hooks, + ); } // Resolve config values before building diff --git a/src/backends/llmist/index.ts b/src/backends/llmist/index.ts index 98c91024..9363a4d5 100644 --- a/src/backends/llmist/index.ts +++ b/src/backends/llmist/index.ts @@ -111,6 +111,12 @@ export class LlmistBackend implements AgentBackend { baseBranch: input.project.baseBranch, projectId: input.project.id, cardId: agentInput.cardId, + // Pass resolved hook flags for finish validation (hook-driven instead of agent-type checks) + hooks: { + requiresPR: profile.requiresPR, + requiresReview: profile.requiresReview, + requiresPushedChanges: profile.requiresPushedChanges, + }, // Pass the progress monitor from the adapter so createObserverHooks can call // onIteration/onToolCall/onText — enables progress updates to Trello/GitHub progressMonitor: progressReporter as Parameters< diff --git a/src/cli/session/finish.ts b/src/cli/session/finish.ts index a133ea7d..147c6994 100644 --- a/src/cli/session/finish.ts +++ b/src/cli/session/finish.ts @@ -28,6 +28,7 @@ export default class Finish extends Command { agentType: flags['agent-type'] ?? process.env.CASCADE_AGENT_TYPE ?? null, prCreated: flags['pr-created'], reviewSubmitted: flags['review-submitted'], + hooks: {}, }); if (!result.valid) { diff --git a/src/gadgets/Finish.ts b/src/gadgets/Finish.ts index 1c507216..7f4d8d23 100644 --- a/src/gadgets/Finish.ts +++ b/src/gadgets/Finish.ts @@ -25,6 +25,7 @@ export class Finish extends Gadget({ agentType: state.agentType, prCreated: state.prCreated, reviewSubmitted: state.reviewSubmitted, + hooks: state.hooks, }); if (!result.valid) { diff --git a/src/gadgets/session/core/finish.ts b/src/gadgets/session/core/finish.ts index 59fb9977..78cc4448 100644 --- a/src/gadgets/session/core/finish.ts +++ b/src/gadgets/session/core/finish.ts @@ -1,5 +1,6 @@ import { execSync } from 'node:child_process'; import { githubClient } from '../../../github/client.js'; +import type { SessionHooks } from '../../sessionState.js'; export function hasUncommittedChanges(): boolean { try { @@ -47,6 +48,7 @@ export interface SessionState { agentType: string | null; prCreated: boolean; reviewSubmitted: boolean; + hooks: SessionHooks; } export interface FinishValidationError { @@ -61,38 +63,42 @@ export interface FinishValidationSuccess { export type FinishValidationResult = FinishValidationError | FinishValidationSuccess; export async function validateFinish(state: SessionState): Promise { - if (state.agentType === 'implementation' && !state.prCreated) { + const hooks = state.hooks ?? {}; + + if (hooks.requiresPR && !state.prCreated) { const prUrl = await findPRForCurrentBranch(); if (!prUrl) { return { valid: false, error: - 'Cannot finish implementation session without creating a PR. ' + + 'Cannot finish session without creating a PR. ' + 'You must call CreatePR to submit your changes before calling Finish.', }; } } - if (state.agentType === 'review' && !state.reviewSubmitted) { + if (hooks.requiresReview && !state.reviewSubmitted) { return { valid: false, error: - 'Cannot finish review session without submitting a review. ' + + 'Cannot finish session without submitting a review. ' + 'You must call CreatePRReview to submit your review before calling Finish.', }; } - if (state.agentType === 'respond-to-review' || state.agentType === 'respond-to-ci') { + if (hooks.requiresPushedChanges) { if (hasUncommittedChanges()) { return { valid: false, - error: `Cannot finish ${state.agentType} session with uncommitted changes. You must commit your changes (git add && git commit) before calling Finish.`, + error: + 'Cannot finish session with uncommitted changes. You must commit your changes (git add && git commit) before calling Finish.', }; } if (hasUnpushedCommits()) { return { valid: false, - error: `Cannot finish ${state.agentType} session without pushing changes. You must push your commits (git push) before calling Finish.`, + error: + 'Cannot finish session without pushing changes. You must push your commits (git push) before calling Finish.', }; } } diff --git a/src/gadgets/sessionState.ts b/src/gadgets/sessionState.ts index 47d520fa..03228388 100644 --- a/src/gadgets/sessionState.ts +++ b/src/gadgets/sessionState.ts @@ -1,3 +1,16 @@ +/** + * SCM hook flags resolved from the agent definition's hooks.scm configuration. + * These drive finish validation logic. + */ +export interface SessionHooks { + /** Whether the agent must create a PR before finishing */ + requiresPR?: boolean; + /** Whether the agent must submit a review before finishing */ + requiresReview?: boolean; + /** Whether the agent must have pushed changes before finishing */ + requiresPushedChanges?: boolean; +} + // Session-level state accessible to all gadgets let sessionState = { agentType: null as string | null, @@ -9,6 +22,7 @@ let sessionState = { reviewSubmitted: false, reviewUrl: null as string | null, initialCommentId: null as number | null, + hooks: {} as SessionHooks, }; export function initSessionState( @@ -16,6 +30,7 @@ export function initSessionState( baseBranch?: string, projectId?: string, cardId?: string, + hooks?: SessionHooks, ): void { sessionState = { agentType, @@ -27,6 +42,7 @@ export function initSessionState( reviewSubmitted: false, reviewUrl: null, initialCommentId: null, + hooks: hooks ?? {}, }; } diff --git a/tests/unit/agents/definitions/loader.test.ts b/tests/unit/agents/definitions/loader.test.ts index 5f6db87f..d938b70a 100644 --- a/tests/unit/agents/definitions/loader.test.ts +++ b/tests/unit/agents/definitions/loader.test.ts @@ -136,15 +136,15 @@ describe('YAML agent definitions loader', () => { expect(def.backend.postConfigure).toBe('sequentialGadgetExecution'); }); - it('implementation has requiresPR flag', () => { + it('implementation has requiresPR flag in hooks.scm', () => { const def = loadAgentDefinition('implementation'); - expect(def.backend.requiresPR).toBe(true); + expect(def.backend.hooks?.scm?.requiresPR).toBe(true); }); - it('non-implementation agents do not have requiresPR', () => { + it('non-implementation agents do not have hooks.scm.requiresPR', () => { for (const agentType of ALL_AGENT_TYPES.filter((t) => t !== 'implementation')) { const def = loadAgentDefinition(agentType); - expect(def.backend.requiresPR).toBeUndefined(); + expect(def.backend.hooks?.scm?.requiresPR).toBeUndefined(); } }); @@ -273,7 +273,7 @@ describe('YAML agent definitions loader', () => { expect(caps.canCreatePR).toBe(true); expect(caps.canUpdateChecklists).toBe(true); expect(caps.isReadOnly).toBe(false); - expect(def.backend.enableStopHooks).toBe(true); + expect(def.backend.hooks?.scm?.enableStopHooks).toBe(true); expect(def.backend.needsGitHubToken).toBe(true); expect(def.backend.preExecute).toBeUndefined(); expect(def.backend.postConfigure).toBe('sequentialGadgetExecution'); @@ -285,7 +285,7 @@ describe('YAML agent definitions loader', () => { expect(caps.canEditFiles).toBe(false); expect(caps.isReadOnly).toBe(true); - expect(def.backend.enableStopHooks).toBe(false); + expect(def.backend.hooks?.scm?.enableStopHooks).toBe(false); expect(def.backend.needsGitHubToken).toBe(true); expect(def.backend.preExecute).toBe('postInitialPRComment'); }); diff --git a/tests/unit/agents/shared/builderFactory.test.ts b/tests/unit/agents/shared/builderFactory.test.ts index f9426b1d..914ca02a 100644 --- a/tests/unit/agents/shared/builderFactory.test.ts +++ b/tests/unit/agents/shared/builderFactory.test.ts @@ -169,6 +169,7 @@ describe('createConfiguredBuilder', () => { undefined, undefined, undefined, + undefined, ); }); @@ -190,6 +191,7 @@ describe('createConfiguredBuilder', () => { 'main', 'project-1', 'card-123', + undefined, ); }); diff --git a/tests/unit/gadgets/finish.test.ts b/tests/unit/gadgets/finish.test.ts index a3e571c3..c9272cd0 100644 --- a/tests/unit/gadgets/finish.test.ts +++ b/tests/unit/gadgets/finish.test.ts @@ -28,15 +28,15 @@ describe('Finish gadget', () => { expect(gadget.exclusive).toBe(true); }); - it('throws TaskCompletionSignal when no agent type is set', async () => { + it('throws TaskCompletionSignal when no hooks are set', async () => { initSessionState('unknown'); const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow(TaskCompletionSignal); }); - describe('implementation agent', () => { + describe('implementation agent (hooks.requiresPR: true)', () => { beforeEach(() => { - initSessionState('implementation'); + initSessionState('implementation', undefined, undefined, undefined, { requiresPR: true }); }); it('rejects finish without PR creation and no PR on branch', async () => { @@ -49,7 +49,7 @@ describe('Finish gadget', () => { const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow( - 'Cannot finish implementation session without creating a PR', + 'Cannot finish session without creating a PR', ); }); @@ -82,14 +82,16 @@ describe('Finish gadget', () => { const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow( - 'Cannot finish implementation session without creating a PR', + 'Cannot finish session without creating a PR', ); }); }); - describe('respond-to-ci agent', () => { + describe('respond-to-ci agent (hooks.requiresPushedChanges: true)', () => { beforeEach(() => { - initSessionState('respond-to-ci'); + initSessionState('respond-to-ci', undefined, undefined, undefined, { + requiresPushedChanges: true, + }); }); it('rejects finish with uncommitted changes', async () => { @@ -100,7 +102,7 @@ describe('Finish gadget', () => { const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow( - 'Cannot finish respond-to-ci session with uncommitted changes', + 'Cannot finish session with uncommitted changes', ); }); @@ -113,7 +115,7 @@ describe('Finish gadget', () => { const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow( - 'Cannot finish respond-to-ci session without pushing changes', + 'Cannot finish session without pushing changes', ); }); @@ -129,15 +131,15 @@ describe('Finish gadget', () => { }); }); - describe('review agent', () => { + describe('review agent (hooks.requiresReview: true)', () => { beforeEach(() => { - initSessionState('review'); + initSessionState('review', undefined, undefined, undefined, { requiresReview: true }); }); it('rejects finish without submitting a review', async () => { const gadget = new Finish(); await expect(gadget.execute({ comment: 'Done' })).rejects.toThrow( - 'Cannot finish review session without submitting a review', + 'Cannot finish session without submitting a review', ); }); diff --git a/tests/unit/gadgets/session/core/finish.test.ts b/tests/unit/gadgets/session/core/finish.test.ts index 0ba207c2..f38f8f34 100644 --- a/tests/unit/gadgets/session/core/finish.test.ts +++ b/tests/unit/gadgets/session/core/finish.test.ts @@ -126,7 +126,8 @@ describe('hasUnpushedCommits', () => { }); describe('validateFinish', () => { - it('implementation + !prCreated + no PR on branch → error', async () => { + // Hook-driven tests: hooks.requiresPR + it('requiresPR + !prCreated + no PR on branch → error', async () => { // findPRForCurrentBranch returns null mockExecSync.mockImplementation(() => { throw new Error('fail'); @@ -136,26 +137,28 @@ describe('validateFinish', () => { agentType: 'implementation', prCreated: false, reviewSubmitted: false, + hooks: { requiresPR: true }, }); expect(result.valid).toBe(false); if (!result.valid) { - expect(result.error).toContain('Cannot finish implementation session'); + expect(result.error).toContain('Cannot finish session without creating a PR'); expect(result.error).toContain('CreatePR'); } }); - it('implementation + prCreated → valid', async () => { + it('requiresPR + prCreated → valid', async () => { const result = await validateFinish({ agentType: 'implementation', prCreated: true, reviewSubmitted: false, + hooks: { requiresPR: true }, }); expect(result.valid).toBe(true); }); - it('implementation + PR found on branch → valid', async () => { + it('requiresPR + PR found on branch → valid', async () => { mockExecSync.mockReturnValueOnce('feat\n').mockReturnValueOnce('https://github.com/o/r.git\n'); mockGithub.getOpenPRByBranch.mockResolvedValue({ @@ -166,42 +169,48 @@ describe('validateFinish', () => { agentType: 'implementation', prCreated: false, reviewSubmitted: false, + hooks: { requiresPR: true }, }); expect(result.valid).toBe(true); }); - it('review + !reviewSubmitted → error', async () => { + // Hook-driven tests: hooks.requiresReview + it('requiresReview + !reviewSubmitted → error', async () => { const result = await validateFinish({ agentType: 'review', prCreated: false, reviewSubmitted: false, + hooks: { requiresReview: true }, }); expect(result.valid).toBe(false); if (!result.valid) { - expect(result.error).toContain('Cannot finish review session'); + expect(result.error).toContain('Cannot finish session without submitting a review'); expect(result.error).toContain('CreatePRReview'); } }); - it('review + reviewSubmitted → valid', async () => { + it('requiresReview + reviewSubmitted → valid', async () => { const result = await validateFinish({ agentType: 'review', prCreated: false, reviewSubmitted: true, + hooks: { requiresReview: true }, }); expect(result.valid).toBe(true); }); - it('respond-to-review + uncommitted → error', async () => { + // Hook-driven tests: hooks.requiresPushedChanges + it('requiresPushedChanges + uncommitted → error', async () => { mockExecSync.mockReturnValue('M dirty.ts'); // has uncommitted changes const result = await validateFinish({ agentType: 'respond-to-review', prCreated: false, reviewSubmitted: false, + hooks: { requiresPushedChanges: true }, }); expect(result.valid).toBe(false); @@ -210,7 +219,7 @@ describe('validateFinish', () => { } }); - it('respond-to-review + unpushed → error', async () => { + it('requiresPushedChanges + unpushed → error', async () => { mockExecSync .mockReturnValueOnce('') // no uncommitted (git status) .mockReturnValueOnce('2\n'); // unpushed commits @@ -219,6 +228,7 @@ describe('validateFinish', () => { agentType: 'respond-to-review', prCreated: false, reviewSubmitted: false, + hooks: { requiresPushedChanges: true }, }); expect(result.valid).toBe(false); @@ -227,7 +237,7 @@ describe('validateFinish', () => { } }); - it('respond-to-review + clean → valid', async () => { + it('requiresPushedChanges + clean → valid', async () => { mockExecSync .mockReturnValueOnce('') // no uncommitted .mockReturnValueOnce('0\n'); // no unpushed @@ -236,18 +246,44 @@ describe('validateFinish', () => { agentType: 'respond-to-review', prCreated: false, reviewSubmitted: false, + hooks: { requiresPushedChanges: true }, }); expect(result.valid).toBe(true); }); - it('respond-to-ci + uncommitted → error', async () => { + // No hooks set → always valid + it('no hooks → valid for any agent type', async () => { + const result = await validateFinish({ + agentType: 'splitting', + prCreated: false, + reviewSubmitted: false, + hooks: {}, + }); + + expect(result.valid).toBe(true); + }); + + it('empty hooks → valid even with incomplete state', async () => { + const result = await validateFinish({ + agentType: 'planning', + prCreated: false, + reviewSubmitted: false, + hooks: {}, + }); + + expect(result.valid).toBe(true); + }); + + // requiresPushedChanges + clean for ci agent (mirrors respond-to-ci) + it('requiresPushedChanges + uncommitted for ci-style agent → error', async () => { mockExecSync.mockReturnValue('M file.ts'); const result = await validateFinish({ agentType: 'respond-to-ci', prCreated: false, reviewSubmitted: false, + hooks: { requiresPushedChanges: true }, }); expect(result.valid).toBe(false); @@ -256,7 +292,7 @@ describe('validateFinish', () => { } }); - it('respond-to-ci + clean → valid', async () => { + it('requiresPushedChanges + clean for ci-style agent → valid', async () => { mockExecSync .mockReturnValueOnce('') // no uncommitted .mockReturnValueOnce('0\n'); // no unpushed @@ -265,16 +301,7 @@ describe('validateFinish', () => { agentType: 'respond-to-ci', prCreated: false, reviewSubmitted: false, - }); - - expect(result.valid).toBe(true); - }); - - it('other agent types → valid', async () => { - const result = await validateFinish({ - agentType: 'splitting', - prCreated: false, - reviewSubmitted: false, + hooks: { requiresPushedChanges: true }, }); expect(result.valid).toBe(true); diff --git a/web/src/components/settings/agent-definition-editor.tsx b/web/src/components/settings/agent-definition-editor.tsx index 1fcc9fe7..f72e3b38 100644 --- a/web/src/components/settings/agent-definition-editor.tsx +++ b/web/src/components/settings/agent-definition-editor.tsx @@ -380,37 +380,76 @@ function BackendSection({ def: AgentDefinition; setBackend: (k: keyof AgentDefinition['backend'], v: unknown) => void; }) { + // Helper to update a specific SCM hook field + const setHook = ( + k: keyof NonNullable['scm']>, + v: unknown, + ) => { + setBackend('hooks', { + ...def.backend.hooks, + scm: { + ...def.backend.hooks?.scm, + [k]: v, + }, + }); + }; + + const scm = def.backend.hooks?.scm; + return (

Backend

-
- setBackend('enableStopHooks', v)} - label="Enable Stop Hooks" - description="Checks for uncommitted/unpushed changes before agent finishes. Enable for implementation; disable for planning/review." - /> + + {/* SCM Hooks */} +
+
SCM Hooks
+
+ setHook('enableStopHooks', v)} + label="Enable Stop Hooks" + description="Checks for uncommitted/unpushed changes before agent finishes. Enable for implementation; disable for planning/review." + /> + setHook('blockGitPush', v)} + label="Block Git Push" + description="Prevents direct pushes, requiring cascade-tools for PRs. Disable for existing PR branches." + /> + setHook('requiresPR', v)} + label="Requires PR" + description="Agent must create a PR before the session can finish." + /> + setHook('requiresReview', v)} + label="Requires Review" + description="Agent must submit a code review before the session can finish." + /> + setHook('requiresPushedChanges', v)} + label="Requires Pushed Changes" + description="Agent must commit and push changes before the session can finish." + /> +
+
+ + {/* Backend Settings */} +
+
Backend Settings
setBackend('needsGitHubToken', v)} label="Needs GitHub Token" description="Agent receives GitHub token for API access. Required for PR creation and code reviews." /> - setBackend('blockGitPush', v)} - label="Block Git Push" - description="Prevents direct pushes, requiring cascade-tools for PRs. Disable for existing PR branches." - /> - setBackend('requiresPR', v)} - label="Requires PR" - description="Agent must create a PR for the session to be considered successful." - />
+
@@ -975,7 +1014,7 @@ const EMPTY_DEFINITION: AgentDefinition = { }, triggers: [], strategies: {}, - backend: { enableStopHooks: false, needsGitHubToken: false }, + backend: { needsGitHubToken: false }, hint: '', trailingMessage: undefined, prompts: {