fix(scm): 🐛 address commit message generator review feedback

Author: Mirrowel

src/services/commit-message/CommitMessageGenerator.ts

@@ -8,26 +8,38 @@ import { TelemetryEventName, type ProviderSettings } from "@roo-code/types"
 
 import { GenerateMessageParams, PromptOptions, ProgressUpdate } from "./types/core"
 
+/** Provides the extension settings needed to generate commit messages. */
 export interface CommitMessageContextProxy {
+	/** Whether the underlying extension configuration is ready to read. */
 	isInitialized: boolean
+	/** Returns the active provider settings used as the default generation profile. */
 	getProviderSettings(): ProviderSettings
+	/** Reads a persisted extension setting by key. */
 	getValue(key: any): unknown
 }
 
+/** Overrides used to isolate commit message generation in tests and integrations. */
 export interface CommitMessageGeneratorDependencies {
+	/** Supplies the context proxy that owns provider settings and user configuration. */
 	getContextProxy?: () => CommitMessageContextProxy
+	/** Completes the prepared commit-message prompt with the selected provider. */
 	completePrompt?: (apiConfiguration: ProviderSettings, promptText: string) => Promise<string>
+	/** Adds repository-specific custom instructions to the commit-message prompt. */
 	addCustomInstructions?: typeof defaultAddCustomInstructions
+	/** Records successful commit-message generation telemetry. */
 	captureGenerated?: () => void
+	/** Receives non-fatal generation warnings, such as profile fallback failures. */
 	logger?: Pick<Console, "warn">
 }
 
+/** Builds prompts, selects provider settings, and extracts AI generated commit messages. */
 export class CommitMessageGenerator {
 	private readonly providerSettingsManager: ProviderSettingsManager
 	private readonly dependencies: Required<CommitMessageGeneratorDependencies>
 	private previousGitContext: string | null = null
 	private previousCommitMessage: string | null = null
 
+	/** Creates a generator using the provider settings manager and optional test seams. */
 	constructor(
 		providerSettingsManager: ProviderSettingsManager,
 		dependencies: CommitMessageGeneratorDependencies = {},
@@ -44,10 +56,13 @@ export class CommitMessageGenerator {
 		}
 	}
 
+	/** Generates a commit message for the supplied Git context. */
 	async generateMessage(params: GenerateMessageParams): Promise<string> {
 		const { gitContext, onProgress } = params
 
 		try {
+			this.validateGitContext(gitContext)
+
 			onProgress?.({
 				message: "Generating commit message...",
 				percentage: 75,
@@ -72,6 +87,7 @@ export class CommitMessageGenerator {
 		}
 	}
 
+	/** Creates the final model prompt, including custom and regeneration instructions. */
 	async buildPrompt(gitContext: string, options: PromptOptions, workspacePath: string): Promise<string> {
 		const { customSupportPrompts = {}, previousContext, previousMessage } = options
 
@@ -128,6 +144,7 @@ FINAL REMINDER: Your message MUST be COMPLETELY DIFFERENT from the previous mess
 		}
 	}
 
+	/** Calls the configured AI provider and returns the cleaned commit message text. */
 	private async callAIForCommitMessage(
 		gitContextString: string,
 		workspacePath: string,
@@ -190,9 +207,34 @@ FINAL REMINDER: Your message MUST be COMPLETELY DIFFERENT from the previous mess
 		return this.extractCommitMessage(response)
 	}
 
+	/** Throws when there is no meaningful Git change data to describe. */
+	private validateGitContext(gitContext: string): void {
+		if (!this.hasGitChanges(gitContext)) {
+			throw new Error("No changes to generate a commit message for")
+		}
+	}
+
+	/** Detects whether collected Git context includes at least one changed file. */
+	private hasGitChanges(gitContext: string): boolean {
+		const normalizedContext = gitContext.trim()
+
+		if (!normalizedContext || normalizedContext.includes("(No changes matched selection)")) {
+			return false
+		}
+
+		return (
+			/^diff --git /m.test(normalizedContext) ||
+			/^Binary file /m.test(normalizedContext) ||
+			/^(Added|Modified|Deleted|Renamed|Copied|Updated|Untracked|Unknown) \((staged|unstaged)\): .+$/m.test(
+				normalizedContext,
+			)
+		)
+	}
+
+	/** Cleans formatting wrappers from an AI response without enforcing message style. */
 	private extractCommitMessage(response: string): string {
 		const cleaned = response.trim()
-		const withoutCodeBlocks = cleaned.replace(/```[a-z]*\n|```/g, "")
+		const withoutCodeBlocks = cleaned.replace(/^```[a-zA-Z0-9_-]*\r?\n/, "").replace(/\r?\n```$/, "")
 		const withoutQuotes = withoutCodeBlocks.replace(/^["'`]|["'`]$/g, "")
 		return withoutQuotes.trim()
 	}

src/services/commit-message/__tests__/CommitMessageGenerator.spec.ts

@@ -19,6 +19,7 @@ describe("CommitMessageGenerator", () => {
 	const captureGenerated = vi.fn()
 	const warn = vi.fn()
 
+	/** Creates a generator with mocked provider and configuration dependencies. */
 	const createGenerator = () =>
 		new CommitMessageGenerator(providerSettingsManager as any, {
 			getContextProxy: () => contextProxy,
@@ -50,6 +51,33 @@ describe("CommitMessageGenerator", () => {
 		providerSettingsManager.getProfile.mockResolvedValue({ name: "Commit profile", ...commitConfig })
 	})
 
+	it("fails before progress or AI calls when git context has no changes", async () => {
+		const onProgress = vi.fn()
+		const generator = createGenerator()
+
+		await expect(
+			generator.generateMessage({
+				workspacePath: "/repo",
+				selectedFiles: [],
+				gitContext: `## Git Context
+
+### Full Diff of Staged Changes
+\`\`\`diff
+\`\`\`
+
+### Change Summary
+\`\`\`
+(No changes matched selection)
+\`\`\``,
+				onProgress,
+			}),
+		).rejects.toThrow("No changes to generate a commit message for")
+
+		expect(onProgress).not.toHaveBeenCalled()
+		expect(completePrompt).not.toHaveBeenCalled()
+		expect(captureGenerated).not.toHaveBeenCalled()
+	})
+
 	it("sends the full git context to the LLM and returns cleaned commit text", async () => {
 		const gitContext = `## Git Context
 
@@ -153,4 +181,23 @@ new file mode 100644
 		expect(secondPrompt).toContain('The previous message was: "feat(git): collect git context"')
 		expect(secondPrompt).toContain(gitContext)
 	})
+
+	it("cleans formatting wrappers without enforcing conventional commit format", async () => {
+		completePrompt.mockResolvedValue(`\`\`\`
+Update Git context parsing for staged-only entries
+
+Keep unstaged commit context focused on worktree changes.
+\`\`\``)
+		const generator = createGenerator()
+
+		const message = await generator.generateMessage({
+			workspacePath: "/repo",
+			selectedFiles: ["src/file.ts"],
+			gitContext: "Modified (staged): src/file.ts",
+		})
+
+		expect(message).toBe(`Update Git context parsing for staged-only entries
+
+Keep unstaged commit context focused on worktree changes.`)
+	})
 })

src/services/commit-message/types/core.ts

@@ -1,18 +1,31 @@
+/** Parameters required to generate a commit message for selected Git changes. */
 export interface GenerateMessageParams {
+	/** Absolute workspace path used to resolve repository custom instructions. */
 	workspacePath: string
+	/** File paths included in the Git context used for generation. */
 	selectedFiles: string[]
+	/** Markdown Git context describing the changes to summarize. */
 	gitContext: string
+	/** Optional progress callback for UI updates during generation. */
 	onProgress?: (progress: ProgressUpdate) => void
 }
 
+/** Prompt customization and regeneration context for commit-message prompts. */
 export interface PromptOptions {
+	/** User-defined support prompt templates keyed by prompt type. */
 	customSupportPrompts?: Record<string, string>
+	/** Previous Git context used to detect regeneration for the same changes. */
 	previousContext?: string
+	/** Previous generated message to avoid repeating during regeneration. */
 	previousMessage?: string
 }
 
+/** Incremental status update emitted while generating a commit message. */
 export interface ProgressUpdate {
+	/** Human-readable status message for the current generation step. */
 	message?: string
+	/** Absolute progress percentage for the current generation step. */
 	percentage?: number
+	/** Relative progress increment for the current generation step. */
 	increment?: number
 }