diff --git a/src/backends/shared/nativeToolPrompts.ts b/src/backends/shared/nativeToolPrompts.ts index f56d75ab..d9d9bd21 100644 --- a/src/backends/shared/nativeToolPrompts.ts +++ b/src/backends/shared/nativeToolPrompts.ts @@ -13,7 +13,15 @@ You are operating in a native-tool environment, not a gadget/function-call envir - use the shell tool for all \`cascade-tools ...\`, \`git ...\`, \`rg ...\`, \`fd ...\`, test, lint, and build commands - When the task instructions mention gadget names like \`CreatePR\`, \`PostComment\`, \`UpdateChecklistItem\`, \`Finish\`, \`ReadWorkItem\`, \`TodoUpsert\`, or \`TodoUpdateStatus\`, treat that as a request to run the equivalent real command or tool action, not to print the gadget name. - If you catch yourself composing a pseudo tool call in plain text, stop and use the real tool instead. -- Trello, JIRA, and GitHub attachment URLs require backend authentication. NEVER curl, wget, or HTTP-fetch them — they return an authorization error. Work item images are pre-fetched and available either as images in your conversation context or as files under \`.cascade/context/images/\` — use whichever is present; never fetch the original URLs.`; +- Trello, JIRA, and GitHub attachment URLs require backend authentication. NEVER curl, wget, or HTTP-fetch them — they return an authorization error. Work item images are pre-fetched and available either as images in your conversation context or as files under \`.cascade/context/images/\` — use whichever is present; never fetch the original URLs. + +## Termination protocol + +When you have completed all required side-effects for this task — per the hooks declared on this agent (e.g. commits pushed, PR opened, review submitted, checklist created, PM comment posted) — call the \`Finish\` tool with a one-paragraph summary of what you did. + +- **Do not** run additional verification commands, re-read files, or post additional comments after a successful Finish call. The session ends the moment Finish succeeds (\`TaskCompletionSignal\`), so anything emitted after it is wasted work that the user pays for. +- If Finish returns an error (e.g. "Cannot finish session without pushing changes"), it means a required precondition is not met yet. Fix the precondition (push your branch, submit your review, etc.) and call Finish again. Do not silently keep working hoping the gate will pass on its own — it will not. +- If your task did not require any side-effects (e.g. you investigated and decided no action was needed), still call Finish with a summary explaining what you found. Always end the session deliberately.`; type PromptParamSchema = { type: string; diff --git a/src/gadgets/session/core/finish.ts b/src/gadgets/session/core/finish.ts index c9a2fd22..9cef6b32 100644 --- a/src/gadgets/session/core/finish.ts +++ b/src/gadgets/session/core/finish.ts @@ -161,27 +161,48 @@ export interface FinishValidationSuccess { export type FinishValidationResult = FinishValidationError | FinishValidationSuccess; +/** + * Build a structured validation failure AND emit a WARN log so prod ops + * can grep `docker logs cascade-router | grep "[Finish] validation rejected"` + * to see why an agent is looping on the gate. Without this, MNG-699-class + * incidents (b728fa3e, 2026-05-12) leave no breadcrumb explaining what + * precondition the agent was missing. + */ +function rejectFinish(reason: string, error: string, state: SessionState): FinishValidationError { + logger.warn('[Finish] validation rejected', { + reason, + error, + agentType: state.agentType, + prCreated: state.prCreated, + reviewSubmitted: state.reviewSubmitted, + prBranch: state.prBranch ?? null, + hasInitialHeadSha: !!state.initialHeadSha, + hooks: state.hooks ?? {}, + }); + return { valid: false, error }; +} + function checkPushedChangesHook(state: SessionState): FinishValidationError | null { if (hasUncommittedChanges()) { - return { - valid: false, - error: - 'Cannot finish session with uncommitted changes. You must commit your changes (git add && git commit) before calling Finish.', - }; + return rejectFinish( + 'uncommitted_changes', + 'Cannot finish session with uncommitted changes. You must commit your changes (git add && git commit) before calling Finish.', + state, + ); } if (hasUnpushedCommits(state.prBranch ?? undefined)) { - return { - valid: false, - error: - 'Cannot finish session without pushing changes. You must push your commits (git push) before calling Finish.', - }; + return rejectFinish( + 'unpushed_commits', + 'Cannot finish session without pushing changes. You must push your commits (git push) before calling Finish.', + state, + ); } if (state.initialHeadSha && !hasNewCommits(state.initialHeadSha)) { - return { - valid: false, - error: - 'Cannot finish session without making any changes. You must commit and push at least one change before calling Finish.', - }; + return rejectFinish( + 'no_new_commits', + 'Cannot finish session without making any changes. You must commit and push at least one change before calling Finish.', + state, + ); } return null; } @@ -192,22 +213,22 @@ export async function validateFinish(state: SessionState): Promise { + // Minimal arguments — the per-agent body and tool list don't matter + // for this guard. We only care that the shared rules block is present. + const composed = buildSystemPrompt('AGENT_BODY_PLACEHOLDER', []); + + it('contains the "Termination protocol" heading', () => { + expect(composed).toMatch(/Termination protocol/); + }); + + it('explicitly names the `Finish` gadget the agent must call', () => { + const protocolBlock = composed.split('Termination protocol')[1] ?? ''; + expect(protocolBlock).toMatch(/\bFinish\b/); + }); + + it('forbids further tool calls after Finish succeeds', () => { + const protocolBlock = composed.split('Termination protocol')[1] ?? ''; + expect(protocolBlock).toMatch( + /do not.*after.*finish|no.*tool.*after.*finish|session ends|stops streaming/i, + ); + }); + + it('still includes the per-agent body after the rules block', () => { + // Sanity: the rules don't accidentally drop the caller-supplied body. + expect(composed).toContain('AGENT_BODY_PLACEHOLDER'); + }); +}); diff --git a/tests/unit/gadgets/session/core/finish.test.ts b/tests/unit/gadgets/session/core/finish.test.ts index dae1a274..4fb30a41 100644 --- a/tests/unit/gadgets/session/core/finish.test.ts +++ b/tests/unit/gadgets/session/core/finish.test.ts @@ -495,4 +495,91 @@ describe('validateFinish', () => { expect(result.valid).toBe(true); }); + + // Diagnostic logging regression net — MNG-699 (ucho/PR #400, 2026-05-12): + // without a breadcrumb when validateFinish rejects, we couldn't tell from + // docker logs why respond-to-review `b728fa3e` was looping on the gate. + // Every invalid path must emit a structured WARN that ops can grep. + describe('diagnostic logging on rejection (MNG-699 regression)', () => { + const baseState = { + prCreated: false, + reviewSubmitted: false, + initialHeadSha: null, + prBranch: null, + }; + + it('logs WARN with reason="missing_review" when review-hook fails', async () => { + const result = await validateFinish({ + ...baseState, + agentType: 'review', + hooks: { requiresReview: true }, + }); + expect(result.valid).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[Finish] validation rejected', + expect.objectContaining({ reason: 'missing_review', agentType: 'review' }), + ); + }); + + it('logs WARN with reason="missing_pr" when PR-hook fails', async () => { + mockExecSync + .mockReturnValueOnce('feature/x\n') // git rev-parse + .mockReturnValueOnce('https://github.com/o/r.git\n'); // git remote + mockGithub.getOpenPRByBranch.mockResolvedValue(null); + + const result = await validateFinish({ + ...baseState, + agentType: 'implementation', + hooks: { requiresPR: true }, + }); + expect(result.valid).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[Finish] validation rejected', + expect.objectContaining({ reason: 'missing_pr', agentType: 'implementation' }), + ); + }); + + it('logs WARN with reason="uncommitted_changes" when working tree dirty', async () => { + mockExecSync.mockReturnValue('M src/file.ts'); + + const result = await validateFinish({ + ...baseState, + agentType: 'respond-to-review', + hooks: { requiresPushedChanges: true }, + }); + expect(result.valid).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[Finish] validation rejected', + expect.objectContaining({ reason: 'uncommitted_changes' }), + ); + }); + + it('logs WARN with reason="unpushed_commits" when commits not pushed', async () => { + mockExecSync + .mockReturnValueOnce('') // no uncommitted + .mockReturnValueOnce('2\n'); // 2 unpushed (rev-list count) + + const result = await validateFinish({ + ...baseState, + agentType: 'respond-to-review', + prBranch: 'feature/MNG-699', + hooks: { requiresPushedChanges: true }, + }); + expect(result.valid).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[Finish] validation rejected', + expect.objectContaining({ reason: 'unpushed_commits', prBranch: 'feature/MNG-699' }), + ); + }); + + it('does NOT log when validation passes', async () => { + const result = await validateFinish({ + ...baseState, + agentType: 'splitting', + hooks: {}, + }); + expect(result.valid).toBe(true); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); + }); });