Merge dev to main

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;

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<FinishValidat
 	if (hooks.requiresPR && !state.prCreated) {
 		const prUrl = await findPRForCurrentBranch();
 		if (!prUrl) {
-			return {
-				valid: false,
-				error:
-					'Cannot finish session without creating a PR. ' +
+			return rejectFinish(
+				'missing_pr',
+				'Cannot finish session without creating a PR. ' +
 					'You must call CreatePR to submit your changes before calling Finish.',
-			};
+				state,
+			);
 		}
 	}
 
 	if (hooks.requiresReview && !state.reviewSubmitted) {
-		return {
-			valid: false,
-			error:
-				'Cannot finish session without submitting a review. ' +
+		return rejectFinish(
+			'missing_review',
+			'Cannot finish session without submitting a review. ' +
 				'You must call CreatePRReview to submit your review before calling Finish.',
-		};
+			state,
+		);
 	}
 
 	if (hooks.requiresPushedChanges) {

tests/unit/agents/definitions/termination-protocol.test.ts

@@ -0,0 +1,46 @@
+import { describe, expect, it } from 'vitest';
+import { buildSystemPrompt } from '../../../../src/backends/shared/nativeToolPrompts.js';
+
+/**
+ * Termination protocol guard — the shared native-tool prompt must instruct
+ * every agent (regardless of role) to call `Finish` when work is done and
+ * stop emitting tool calls.
+ *
+ * Background: MNG-699 / ucho PR #400 (2026-05-12) — respond-to-review run
+ * `b728fa3e` finished its real work at 08:43:22 (commit pushed, review reply
+ * posted) but kept emitting tool calls for ~32 more minutes until the user
+ * cancelled at 09:15:08. Root cause: the prompt didn't mandate calling
+ * `Finish`. Without an explicit instruction, the model may decide it is
+ * done by emitting trailing text but never invokes the gadget that throws
+ * `TaskCompletionSignal` — so the SDK keeps streaming.
+ *
+ * Lives in the shared `NATIVE_TOOL_EXECUTION_RULES` block (delivered via
+ * `buildSystemPrompt`) rather than per-agent YAML so every agent inherits
+ * it without per-yaml duplication. New agents pick it up for free.
+ */
+describe('termination protocol in shared system prompt', () => {
+	// 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');
+	});
+});

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();
+		});
+	});
 });