From 561f07ff07d58f39e85eb71fa158ad37227056fb Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Thu, 13 Aug 2026 15:56:07 +0200 Subject: [PATCH 01/46] refactor(review): make the AI review engine diff-source-agnostic Lift indexDiffFiles/parseFileDiff/parseUnifiedDiff out of usePrPanel.ts into a pure utils/unifiedDiff.ts (no Vue, no backend import), and isEditableTarget out of usePrReviewKeymap.ts into utils/editableTarget.ts, both re-exported for back-compat so every pre-existing test keeps passing unmodified. usePrPreReview's analyzeFile gains a scope: "pr" | "commit" option that swaps only the prompt's framing and dependency-signal sentences; severity scale, confidence rules and the JSON contract stay byte-identical. This lets the v3.7.0 commit-review feature reuse the same engine over the staged diff instead of a PR, with no behavior change for existing PR callers (default stays "pr"). --- .../__tests__/usePrPreReview.test.ts | 24 +++++ apps/desktop/src/composables/usePrPanel.ts | 92 ++---------------- .../desktop/src/composables/usePrPreReview.ts | 24 ++++- .../src/composables/usePrReviewKeymap.ts | 22 ++--- .../src/utils/__tests__/unifiedDiff.test.ts | 69 ++++++++++++++ apps/desktop/src/utils/editableTarget.ts | 25 +++++ apps/desktop/src/utils/unifiedDiff.ts | 94 +++++++++++++++++++ 7 files changed, 249 insertions(+), 101 deletions(-) create mode 100644 apps/desktop/src/utils/__tests__/unifiedDiff.test.ts create mode 100644 apps/desktop/src/utils/editableTarget.ts create mode 100644 apps/desktop/src/utils/unifiedDiff.ts diff --git a/apps/desktop/src/composables/__tests__/usePrPreReview.test.ts b/apps/desktop/src/composables/__tests__/usePrPreReview.test.ts index 9e14a9b6..756d75dd 100644 --- a/apps/desktop/src/composables/__tests__/usePrPreReview.test.ts +++ b/apps/desktop/src/composables/__tests__/usePrPreReview.test.ts @@ -182,4 +182,28 @@ describe("usePrPreReview.analyzeFile", () => { const [, userPrompt] = rawPromptMock.mock.calls[0]; expect(userPrompt).toContain("carol: initial impl"); }); + + // Task 0 (v3.7.0) — the engine's prompt is scope-parametrized so + // `useCommitReview` (v3.7.0) can reuse it over the staged diff instead of + // a PR. Default is unchanged ("pr"); severity scale, confidence rules, and + // JSON contract stay byte-identical across scopes — only the framing + // sentence and the dependency-signal sentence change. + it("defaults to the pull-request framing when scope is omitted", async () => { + rawPromptMock.mockResolvedValue("[]"); + const { analyzeFile } = usePrPreReview(); + const file = diffFile("a.ts", [{ type: "add", content: "x", newLineNo: 1 }]); + await analyzeFile(file, { cwd: "/repo", otherDiffFiles: [] }); + const [systemPrompt] = rawPromptMock.mock.calls[0]; + expect(systemPrompt).toContain("pull request"); + }); + + it("uses the staged-commit framing (and never 'pull request') when scope is 'commit'", async () => { + rawPromptMock.mockResolvedValue("[]"); + const { analyzeFile } = usePrPreReview(); + const file = diffFile("a.ts", [{ type: "add", content: "x", newLineNo: 1 }]); + await analyzeFile(file, { cwd: "/repo", otherDiffFiles: [], scope: "commit" }); + const [systemPrompt] = rawPromptMock.mock.calls[0]; + expect(systemPrompt).not.toContain("pull request"); + expect(systemPrompt).toContain("staged changes"); + }); }); diff --git a/apps/desktop/src/composables/usePrPanel.ts b/apps/desktop/src/composables/usePrPanel.ts index e39f4799..d4961aa2 100644 --- a/apps/desktop/src/composables/usePrPanel.ts +++ b/apps/desktop/src/composables/usePrPanel.ts @@ -16,8 +16,6 @@ import { type CIAnnotation, type RemoteInfo, type GitDiff, - type DiffHunk, - type DiffLine, type PrReviewComment, type CreatePrCommentParams, type PendingReviewComment, @@ -38,6 +36,14 @@ import { getPersistedDiffMode, type DiffMode } from "../utils/diffMode"; import { requireOnline } from "../utils/networkGuard"; import { t } from "./useI18n"; import { useReviewIntelligence } from "./useReviewIntelligence"; +import { indexDiffFiles, parseFileDiff, parseUnifiedDiff } from "../utils/unifiedDiff"; + +// Task 0 (v3.7.0) — the three pure diff-parsing helpers below now live in +// `utils/unifiedDiff.ts` (no Vue, no backend import) so a commit-review +// composable never needs to import this (very large) PR panel just to parse +// a diff. Re-exported here verbatim for existing callers +// (`useReviewIntelligence.ts`, `__tests__/usePrPanel-lazy-diff.test.ts`). +export { indexDiffFiles, parseFileDiff, parseUnifiedDiff }; export const PR_PANEL_KEY = Symbol("prPanel"); @@ -88,86 +94,8 @@ export interface PrPanelOptions { // headers (cheap), and `parseFileDiff` — the actual hunk/line parse — runs // only for the file currently selected (see `ensureFileParsed` below), cached // by path so re-selecting a file never re-parses it. Both are pure (no -// composable state) so they're testable in isolation. - -/** Split a raw unified diff into lightweight per-file slices (no hunk parse). */ -export function indexDiffFiles(rawDiff: string): { path: string; raw: string }[] { - const slices: { path: string; raw: string }[] = []; - if (!rawDiff.trim()) return slices; - const lines = rawDiff.split("\n"); - let currentPath: string | null = null; - let currentLines: string[] = []; - const flush = () => { - if (currentPath !== null) slices.push({ path: currentPath, raw: currentLines.join("\n") }); - }; - for (const line of lines) { - if (line.startsWith("diff --git ")) { - flush(); - const match = line.match(/diff --git a\/(.+) b\/(.+)/); - currentPath = match ? match[2] : "unknown"; - currentLines = [line]; - continue; - } - if (currentPath !== null) currentLines.push(line); - } - flush(); - return slices; -} - -/** Parse one file's raw `diff --git …` slice (as produced by `indexDiffFiles`) - * into hunks/lines. Diff-parsing gotcha (AGENTS.md): context lines are - * detected via `line.startsWith(' ')` — a bare empty string is also treated - * as a (whitespace-stripped) context line, never as a phantom add/delete. */ -export function parseFileDiff(rawFileSlice: string): GitDiff { - const file: GitDiff = { path: "unknown", hunks: [] }; - let currentHunk: DiffHunk | null = null; - let oldLine = 0, newLine = 0; - for (const line of rawFileSlice.split("\n")) { - if (line.startsWith("diff --git ")) { - const match = line.match(/diff --git a\/(.+) b\/(.+)/); - file.path = match ? match[2] : "unknown"; - currentHunk = null; - continue; - } - if (line.startsWith("index ") || line.startsWith("--- ") || line.startsWith("+++ ") || - line.startsWith("old mode ") || line.startsWith("new mode ") || line.startsWith("new file ") || - line.startsWith("deleted file ") || line.startsWith("similarity index ") || - line.startsWith("rename from ") || line.startsWith("rename to ") || line.startsWith("Binary files ")) continue; - const hunkMatch = line.match(/^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)/); - if (hunkMatch) { - currentHunk = { - header: line, - oldStart: parseInt(hunkMatch[1], 10), - oldCount: parseInt(hunkMatch[2] ?? "1", 10), - newStart: parseInt(hunkMatch[3], 10), - newCount: parseInt(hunkMatch[4] ?? "1", 10), - lines: [], - }; - file.hunks.push(currentHunk); - oldLine = parseInt(hunkMatch[1], 10); - newLine = parseInt(hunkMatch[3], 10); - continue; - } - if (currentHunk) { - if (line.startsWith("+")) { - currentHunk.lines.push({ type: "add", content: line.substring(1), newLineNo: newLine++ }); - } else if (line.startsWith("-")) { - currentHunk.lines.push({ type: "delete", content: line.substring(1), oldLineNo: oldLine++ }); - } else if (line.startsWith(" ") || line === "") { - currentHunk.lines.push({ type: "context", content: line.startsWith(" ") ? line.substring(1) : line, oldLineNo: oldLine++, newLineNo: newLine++ }); - } - } - } - return file; -} - -/** Full eager parse — composed from `indexDiffFiles` + `parseFileDiff`. - * Not used on the hot path anymore (see `ensureFileParsed`); kept for - * regression-parity tests and any caller that genuinely wants everything - * parsed up front. */ -export function parseUnifiedDiff(rawDiff: string): GitDiff[] { - return indexDiffFiles(rawDiff).map((f) => parseFileDiff(f.raw)); -} +// composable state) — moved to `utils/unifiedDiff.ts` (Task 0, v3.7.0) and +// re-exported above. export function usePrPanel(cwd: Ref, opts: PrPanelOptions = {}) { diff --git a/apps/desktop/src/composables/usePrPreReview.ts b/apps/desktop/src/composables/usePrPreReview.ts index 5d192fe3..ba4df3d5 100644 --- a/apps/desktop/src/composables/usePrPreReview.ts +++ b/apps/desktop/src/composables/usePrPreReview.ts @@ -35,6 +35,15 @@ export interface ReviewFinding { detail: string; } +/** + * Task 0 (v3.7.0) — which diff source `analyzeFile` is reviewing. Swaps only + * the prompt's framing sentence and dependency-signal sentence; severity + * scale, confidence rules, and JSON contract stay byte-identical across + * scopes — it's the same engine either way. Default `"pr"` — no behavior + * change for any existing PR caller. + */ +export type ReviewScope = "pr" | "commit"; + export interface AnalyzeFileOptions { cwd: string; /** UI locale — drives the response language. */ @@ -46,6 +55,8 @@ export interface AnalyzeFileOptions { * a bigger budget than the single-hunk critique since findings are * file-scoped, not hunk-scoped). */ maxFileChars?: number; + /** Which diff source is being reviewed (Task 0, v3.7.0). Default `"pr"`. */ + scope?: ReviewScope; } // ─── Hop 2: dependency (extraction + cross-reference, no graph) ─────────── @@ -120,14 +131,19 @@ export function summarizeBlameForFile(blame: BlameLine[], file: GitDiff): string // ─── Hop 4: findings (strict JSON, defensive parse) ─────────────────────── -function buildSystemPrompt(locale: string): string { +function buildSystemPrompt(locale: string, scope: ReviewScope = "pr"): string { const lang = localeToAiLanguage(locale); - return `You are a senior engineer doing a pre-review pass on one file of a pull request. + const framing = + scope === "commit" + ? "one file of a commit that is about to be created (the staged changes)" + : "one file of a pull request"; + const dependencyScope = scope === "commit" ? "other files staged in this same commit" : "other files in this same PR"; + return `You are a senior engineer doing a pre-review pass on ${framing}. You will receive: - The file path. - The file's touched hunks (unified-diff content). -- A dependency signal: how many other files in this same PR import this file. +- A dependency signal: how many ${dependencyScope} import this file. - A brief history signal: recent commit summaries that touched the lines now being changed. @@ -266,7 +282,7 @@ export function usePrPreReview() { const blame = await getGitBlame(opts.cwd, file.path).catch(() => [] as BlameLine[]); const blameSummaries = summarizeBlameForFile(blame, file); - const systemPrompt = buildSystemPrompt(opts.locale ?? "en"); + const systemPrompt = buildSystemPrompt(opts.locale ?? "en", opts.scope ?? "pr"); const userPrompt = buildUserPrompt(file, importedByCount, blameSummaries, opts.maxFileChars ?? 6000); const raw = await ai.rawPrompt(systemPrompt, userPrompt); if (!raw) return []; diff --git a/apps/desktop/src/composables/usePrReviewKeymap.ts b/apps/desktop/src/composables/usePrReviewKeymap.ts index bb0ad4b4..5affc7c0 100644 --- a/apps/desktop/src/composables/usePrReviewKeymap.ts +++ b/apps/desktop/src/composables/usePrReviewKeymap.ts @@ -8,6 +8,13 @@ * owns the listener and dispatches. */ +import { isEditableTarget } from "../utils/editableTarget"; + +// Task 0 (v3.7.0) — `isEditableTarget` now lives in `utils/editableTarget.ts` +// (no Vue, no PR-review coupling). Re-exported here verbatim — existing +// tests (`usePrReviewKeymap.test.ts`) import it from this module unmodified. +export { isEditableTarget }; + export type PrReviewAction = | "next-hunk" | "prev-hunk" @@ -22,21 +29,6 @@ export type PrReviewAction = | "prev-finding" | "submit-review"; -/** True when `el` is a text input, textarea, select, or contenteditable — - * the keymap must stay completely inert while the user is typing there. */ -export function isEditableTarget(el: EventTarget | null): boolean { - if (!(el instanceof HTMLElement)) return false; - const tag = el.tagName; - if (tag === "INPUT" || tag === "TEXTAREA" || tag === "SELECT") return true; - // `isContentEditable` computes inherited editability but jsdom doesn't - // fully implement it — check the attribute directly too so this guard is - // reliable in both the browser and the test environment. - if (el.isContentEditable) return true; - const attr = el.getAttribute("contenteditable"); - if (attr === "" || attr === "true") return true; - return false; -} - /** * Resolve a keydown into a PR-review action, or `null` when the key is * unmapped, the panel isn't focused, or an unrelated modifier is held. diff --git a/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts b/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts new file mode 100644 index 00000000..50f6ca95 --- /dev/null +++ b/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts @@ -0,0 +1,69 @@ +/** + * Task 0 (v3.7.0) — `indexDiffFiles` / `parseFileDiff` / `parseUnifiedDiff` lifted + * out of `usePrPanel.ts` into a pure, Vue-free, backend-free module so a + * commit-review composable never has to import the PR panel to parse a diff. + * + * These three assertions are copied from `usePrPanel-lazy-diff.test.ts` — the + * blank-context-line one is the diff-parsing gotcha guard (AGENTS.md): an + * empty string is a context line, never a phantom add/delete. + */ +import { describe, it, expect } from "vitest"; +import { indexDiffFiles, parseFileDiff, parseUnifiedDiff } from "../unifiedDiff"; + +const THREE_FILE_DIFF = [ + "diff --git a/a.ts b/a.ts", + "index 111..222 100644", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1,2 +1,2 @@", + " context a", + "-old a", + "+new a", + "diff --git a/b.ts b/b.ts", + "index 333..444 100644", + "--- a/b.ts", + "+++ b/b.ts", + "@@ -1,3 +1,3 @@", + " context b", + "", + "-old b", + "+new b", + "diff --git a/c.ts b/c.ts", + "index 555..666 100644", + "--- a/c.ts", + "+++ b/c.ts", + "@@ -1,1 +1,1 @@", + "-old c", + "+new c", +].join("\n"); + +describe("indexDiffFiles", () => { + it("splits a raw multi-file diff into per-file slices with correct b/ paths", () => { + const slices = indexDiffFiles(THREE_FILE_DIFF); + expect(slices.map((s) => s.path)).toEqual(["a.ts", "b.ts", "c.ts"]); + for (const s of slices) { + expect(s.raw.startsWith(`diff --git a/${s.path} b/${s.path}`)).toBe(true); + } + }); + + it("returns [] for an empty diff", () => { + expect(indexDiffFiles("")).toEqual([]); + expect(indexDiffFiles(" ")).toEqual([]); + }); +}); + +describe("parseFileDiff", () => { + it("matches parseUnifiedDiff's per-file output (regression parity)", () => { + const expected = parseUnifiedDiff(THREE_FILE_DIFF); + const slices = indexDiffFiles(THREE_FILE_DIFF); + const actual = slices.map((s) => parseFileDiff(s.raw)); + expect(actual).toEqual(expected); + }); + + it("classifies an empty-string context line as context, not add/delete", () => { + const slice = indexDiffFiles(THREE_FILE_DIFF)[1]; // b.ts has a blank context line + const parsed = parseFileDiff(slice.raw); + const blank = parsed.hunks[0].lines.find((l) => l.content === "" && l.type !== undefined); + expect(blank?.type).toBe("context"); + }); +}); diff --git a/apps/desktop/src/utils/editableTarget.ts b/apps/desktop/src/utils/editableTarget.ts new file mode 100644 index 00000000..1a62928f --- /dev/null +++ b/apps/desktop/src/utils/editableTarget.ts @@ -0,0 +1,25 @@ +/** + * editableTarget.ts + * + * Task 0 (v3.7.0) — `isEditableTarget` lifted verbatim out of + * `usePrReviewKeymap.ts` (B1, v3.6.0) so any keymap resolver (PR review, + * commit review, …) can reuse the same "is the user typing here" guard + * without depending on the PR-review keymap module. Re-exported from + * `usePrReviewKeymap.ts` for back-compat — its existing tests import it + * from there unmodified. + */ + +/** True when `el` is a text input, textarea, select, or contenteditable — + * the keymap must stay completely inert while the user is typing there. */ +export function isEditableTarget(el: EventTarget | null): boolean { + if (!(el instanceof HTMLElement)) return false; + const tag = el.tagName; + if (tag === "INPUT" || tag === "TEXTAREA" || tag === "SELECT") return true; + // `isContentEditable` computes inherited editability but jsdom doesn't + // fully implement it — check the attribute directly too so this guard is + // reliable in both the browser and the test environment. + if (el.isContentEditable) return true; + const attr = el.getAttribute("contenteditable"); + if (attr === "" || attr === "true") return true; + return false; +} diff --git a/apps/desktop/src/utils/unifiedDiff.ts b/apps/desktop/src/utils/unifiedDiff.ts new file mode 100644 index 00000000..85acbf83 --- /dev/null +++ b/apps/desktop/src/utils/unifiedDiff.ts @@ -0,0 +1,94 @@ +/** + * unifiedDiff.ts + * + * Task 0 (v3.7.0) — pure unified-diff parsing helpers, lifted verbatim out of + * `usePrPanel.ts` (A1, v3.6.0) so any composable can index/parse a raw + * unified diff without importing the (very large) PR panel. No Vue, no + * backend import — safe to use from `useCommitReview.ts` and anywhere else + * that just needs `GitDiff`s out of a raw diff string. + * + * `usePrPanel.ts` re-exports these three functions for backward + * compatibility — existing callers (`useReviewIntelligence.ts`, + * `usePrPanel-lazy-diff.test.ts`) keep importing from `../usePrPanel` + * unmodified. + */ +import type { GitDiff, DiffHunk } from "./backend"; + +/** Split a raw unified diff into lightweight per-file slices (no hunk parse). */ +export function indexDiffFiles(rawDiff: string): { path: string; raw: string }[] { + const slices: { path: string; raw: string }[] = []; + if (!rawDiff.trim()) return slices; + const lines = rawDiff.split("\n"); + let currentPath: string | null = null; + let currentLines: string[] = []; + const flush = () => { + if (currentPath !== null) slices.push({ path: currentPath, raw: currentLines.join("\n") }); + }; + for (const line of lines) { + if (line.startsWith("diff --git ")) { + flush(); + const match = line.match(/diff --git a\/(.+) b\/(.+)/); + currentPath = match ? match[2] : "unknown"; + currentLines = [line]; + continue; + } + if (currentPath !== null) currentLines.push(line); + } + flush(); + return slices; +} + +/** Parse one file's raw `diff --git …` slice (as produced by `indexDiffFiles`) + * into hunks/lines. Diff-parsing gotcha (AGENTS.md): context lines are + * detected via `line.startsWith(' ')` — a bare empty string is also treated + * as a (whitespace-stripped) context line, never as a phantom add/delete. */ +export function parseFileDiff(rawFileSlice: string): GitDiff { + const file: GitDiff = { path: "unknown", hunks: [] }; + let currentHunk: DiffHunk | null = null; + let oldLine = 0, newLine = 0; + for (const line of rawFileSlice.split("\n")) { + if (line.startsWith("diff --git ")) { + const match = line.match(/diff --git a\/(.+) b\/(.+)/); + file.path = match ? match[2] : "unknown"; + currentHunk = null; + continue; + } + if (line.startsWith("index ") || line.startsWith("--- ") || line.startsWith("+++ ") || + line.startsWith("old mode ") || line.startsWith("new mode ") || line.startsWith("new file ") || + line.startsWith("deleted file ") || line.startsWith("similarity index ") || + line.startsWith("rename from ") || line.startsWith("rename to ") || line.startsWith("Binary files ")) continue; + const hunkMatch = line.match(/^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@(.*)/); + if (hunkMatch) { + currentHunk = { + header: line, + oldStart: parseInt(hunkMatch[1], 10), + oldCount: parseInt(hunkMatch[2] ?? "1", 10), + newStart: parseInt(hunkMatch[3], 10), + newCount: parseInt(hunkMatch[4] ?? "1", 10), + lines: [], + }; + file.hunks.push(currentHunk); + oldLine = parseInt(hunkMatch[1], 10); + newLine = parseInt(hunkMatch[3], 10); + continue; + } + if (currentHunk) { + if (line.startsWith("+")) { + currentHunk.lines.push({ type: "add", content: line.substring(1), newLineNo: newLine++ }); + } else if (line.startsWith("-")) { + currentHunk.lines.push({ type: "delete", content: line.substring(1), oldLineNo: oldLine++ }); + } else if (line.startsWith(" ") || line === "") { + currentHunk.lines.push({ type: "context", content: line.startsWith(" ") ? line.substring(1) : line, oldLineNo: oldLine++, newLineNo: newLine++ }); + } + } + } + return file; +} + +/** Full eager parse — composed from `indexDiffFiles` + `parseFileDiff`. + * Not used on the hot path anymore (see `usePrPanel.ensureFileParsed`); kept + * for regression-parity tests and any caller that genuinely wants + * everything parsed up front. */ +export function parseUnifiedDiff(rawDiff: string): GitDiff[] { + return indexDiffFiles(rawDiff).map((f) => parseFileDiff(f.raw)); +} From b691ec3465a9eeab512d481daa918345fb44f50b Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Thu, 13 Aug 2026 16:21:59 +0200 Subject: [PATCH 02/46] feat(commit-review): add staged-diff AI review engine + opt-in setting Add useCommitReview.ts, a composable that runs the v3.6.0 pre-review engine (usePrPreReview.analyzeFile, scope: "commit") over the staged diff fetched via the existing gitExec(["diff", "--cached"]) primitive. No new Tauri command. Reuses the existing usePrReviewQueue for sequential/abortable/visibility-gated execution and the existing reviewAiConfidenceThreshold/reviewAiMaxFindings settings for the threshold and cap, rather than adding commit-specific twins. Caps the reviewed staged set at 40 files / ~400 KB (slice-order truncation) so a huge staged tree cannot fan out hundreds of LLM calls, exposing a truncated flag for the UI. Off by default: add commitReviewEnabled (opt-in) and commitReviewAutoReReview settings to both useSettings.ts and SettingsPanel.vue, with a new "Commit Review" group in the AI tab. Add commitReview.* and settings.commitReview.* i18n keys to all 5 locales. --- apps/desktop/src/components/SettingsPanel.vue | 36 +++ .../__tests__/useCommitReview.test.ts | 185 ++++++++++++++ .../useSettings-commitReview.test.ts | 38 +++ .../src/composables/useCommitReview.ts | 225 ++++++++++++++++++ apps/desktop/src/composables/useSettings.ts | 18 ++ apps/desktop/src/locales/en.ts | 34 +++ apps/desktop/src/locales/es.ts | 34 +++ apps/desktop/src/locales/fr.ts | 34 +++ apps/desktop/src/locales/pt-BR.ts | 34 +++ apps/desktop/src/locales/zh-CN.ts | 34 +++ 10 files changed, 672 insertions(+) create mode 100644 apps/desktop/src/composables/__tests__/useCommitReview.test.ts create mode 100644 apps/desktop/src/composables/__tests__/useSettings-commitReview.test.ts create mode 100644 apps/desktop/src/composables/useCommitReview.ts diff --git a/apps/desktop/src/components/SettingsPanel.vue b/apps/desktop/src/components/SettingsPanel.vue index 36ee5b56..cf2578a6 100644 --- a/apps/desktop/src/components/SettingsPanel.vue +++ b/apps/desktop/src/components/SettingsPanel.vue @@ -222,6 +222,9 @@ interface Settings { // v3.5.0 Secrets scanner secretsScannerEnabled: boolean; secretsEntropyThreshold: number; + // v3.7.0 Commit Review + commitReviewEnabled: boolean; + commitReviewAutoReReview: boolean; } const defaultSettings: Settings = { @@ -309,6 +312,8 @@ const defaultSettings: Settings = { filesHideOnNav: true, secretsScannerEnabled: true, secretsEntropyThreshold: 4.0, + commitReviewEnabled: false, + commitReviewAutoReReview: true, }; function loadSettings(): Settings { @@ -2745,6 +2750,37 @@ function deleteReleaseNoteTemplate(id: string) { + +
+
+
+
+ {{ t('settings.commitReview.title') }} + {{ t('settings.commitReview.hint') }} +
+
+ +
+ + {{ t('settings.commitReview.enabledHint') }} +
+ +
+ + {{ t('settings.commitReview.autoReReviewHint') }} +
+
+
diff --git a/apps/desktop/src/composables/__tests__/useCommitReview.test.ts b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts new file mode 100644 index 00000000..b21156d4 --- /dev/null +++ b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts @@ -0,0 +1,185 @@ +/** + * Task 1a (v3.7.0) — `useCommitReview` orchestrator: staged-diff AI review + * engine, opt-in and off by default. Mocks `../../utils/backend` and + * `../useAIProvider` per the established convention (`usePrPreReview.test.ts`). + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +const rawPromptMock = vi.fn(); +const isAvailableRef = { value: true }; +const getGitBlameMock = vi.fn(); +const gitExecMock = vi.fn(); + +vi.mock("../useAIProvider", () => ({ + useAIProvider: () => ({ + isAvailable: isAvailableRef, + rawPrompt: (...a: unknown[]) => rawPromptMock(...a), + }), +})); + +vi.mock("../../utils/backend", () => ({ + getGitBlame: (...a: unknown[]) => getGitBlameMock(...a), + gitExec: (...a: unknown[]) => gitExecMock(...a), +})); + +import { useCommitReview, COMMIT_REVIEW_MAX_FILES } from "../useCommitReview"; +import { useSettings, defaultAppSettings } from "../useSettings"; + +function diffFor(path: string): string { + return [ + `diff --git a/${path} b/${path}`, + "index 111..222 100644", + `--- a/${path}`, + `+++ b/${path}`, + "@@ -1,1 +1,1 @@", + "-old", + "+new", + ].join("\n"); +} + +function gitExecOk(stdout: string) { + return { stdout, stderr: "", exitCode: 0 }; +} + +function enableCommitReview(overrides: Partial = {}) { + const { settings } = useSettings(); + settings.value = { ...defaultAppSettings, commitReviewEnabled: true, ...overrides }; +} + +describe("useCommitReview", () => { + beforeEach(() => { + rawPromptMock.mockReset(); + getGitBlameMock.mockReset().mockResolvedValue([]); + gitExecMock.mockReset(); + isAvailableRef.value = true; + const { settings } = useSettings(); + settings.value = { ...defaultAppSettings }; + }); + + it("performs zero IPC and zero LLM calls when the setting is disabled", async () => { + const { settings } = useSettings(); + settings.value = { ...defaultAppSettings, commitReviewEnabled: false }; + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(gitExecMock).not.toHaveBeenCalled(); + expect(rawPromptMock).not.toHaveBeenCalled(); + expect(review.findings.value).toEqual([]); + }); + + it("performs zero IPC and zero LLM calls when the AI provider is unavailable", async () => { + enableCommitReview(); + isAvailableRef.value = false; + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(gitExecMock).not.toHaveBeenCalled(); + expect(rawPromptMock).not.toHaveBeenCalled(); + expect(review.findings.value).toEqual([]); + }); + + it("reviews two staged files and aggregates findings + progress + per-file counts", async () => { + enableCommitReview(); + gitExecMock.mockResolvedValue(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); + rawPromptMock + .mockResolvedValueOnce('[{"line": 1, "title": "finding a", "confidence": 80}]') + .mockResolvedValueOnce('[{"line": 1, "title": "finding b", "confidence": 80}]'); + + const review = useCommitReview(); + await review.run("/repo", "en"); + + expect(review.findings.value).toHaveLength(2); + expect(review.progress.value).toEqual({ done: 2, total: 2 }); + expect(review.findingsByFile.value).toEqual({ "a.ts": 1, "b.ts": 1 }); + }); + + it("sets lastError (never throws) and leaves findings empty when gitExec exits non-zero", async () => { + enableCommitReview(); + gitExecMock.mockResolvedValue({ stdout: "", stderr: "fatal: not a git repository", exitCode: 128 }); + + const review = useCommitReview(); + await expect(review.run("/repo", "en")).resolves.toBeUndefined(); + expect(review.findings.value).toEqual([]); + expect(review.lastError.value).toBeTruthy(); + expect(rawPromptMock).not.toHaveBeenCalled(); + }); + + it("makes no LLM call on an empty staged diff (clean index is not an error)", async () => { + enableCommitReview(); + gitExecMock.mockResolvedValue(gitExecOk("")); + + const review = useCommitReview(); + await review.run("/repo", "en"); + + expect(rawPromptMock).not.toHaveBeenCalled(); + expect(review.findings.value).toEqual([]); + expect(review.lastError.value).toBeNull(); + }); + + it("aborts a run in flight when a second run() starts, painting no stale findings", async () => { + enableCommitReview(); + let resolveFirst!: (v: string) => void; + const pending = new Promise((resolve) => { resolveFirst = resolve; }); + + gitExecMock + .mockResolvedValueOnce(gitExecOk(diffFor("a.ts"))) + .mockResolvedValueOnce(gitExecOk(diffFor("c.ts"))); + rawPromptMock + .mockImplementationOnce(() => pending) + .mockResolvedValueOnce('[{"line": 1, "title": "finding c", "confidence": 80}]'); + + const review = useCommitReview(); + const firstRun = review.run("/repo", "en"); // starts, blocks on rawPrompt for a.ts + // Flush every pending microtask chain (gitExec, queue's waitWhileHidden, + // getGitBlame) so execution actually reaches the blocked rawPrompt call + // before the second run starts — a macrotask tick drains all of them + // since none of those awaits depend on a timer themselves. + await new Promise((resolve) => setTimeout(resolve, 0)); + + const secondRun = review.run("/repo", "en"); // aborts the first, reviews c.ts + await secondRun; + resolveFirst('[{"line": 1, "title": "finding a (stale)", "confidence": 80}]'); + await firstRun; + await Promise.resolve(); + + expect(review.findings.value.map((f) => f.title)).toEqual(["finding c"]); + }); + + it("reset() clears findings, error, and aborts any run in flight", async () => { + enableCommitReview(); + gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue('[{"line": 1, "title": "finding a", "confidence": 80}]'); + + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(review.findings.value).toHaveLength(1); + + review.reset(); + expect(review.findings.value).toEqual([]); + expect(review.rawFindings.value).toEqual([]); + expect(review.lastError.value).toBeNull(); + }); + + it("keeps a below-threshold finding in rawFindings but filters it out of findings", async () => { + enableCommitReview({ reviewAiConfidenceThreshold: 60 }); + gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue('[{"line": 1, "title": "low-confidence finding", "confidence": 20}]'); + + const review = useCommitReview(); + await review.run("/repo", "en"); + + expect(review.findings.value).toEqual([]); + expect(review.rawFindings.value).toHaveLength(1); + }); + + it("caps the staged file count at COMMIT_REVIEW_MAX_FILES and marks truncated", async () => { + enableCommitReview(); + const manyFiles = Array.from({ length: 45 }, (_, i) => diffFor(`f${i}.ts`)).join("\n"); + gitExecMock.mockResolvedValue(gitExecOk(manyFiles)); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview(); + await review.run("/repo", "en"); + + expect(rawPromptMock.mock.calls.length).toBeLessThanOrEqual(COMMIT_REVIEW_MAX_FILES); + expect(review.truncated.value).toBe(true); + }); +}); diff --git a/apps/desktop/src/composables/__tests__/useSettings-commitReview.test.ts b/apps/desktop/src/composables/__tests__/useSettings-commitReview.test.ts new file mode 100644 index 00000000..35aa0bea --- /dev/null +++ b/apps/desktop/src/composables/__tests__/useSettings-commitReview.test.ts @@ -0,0 +1,38 @@ +/** + * Task 1a (v3.7.0) — Commit Review settings: defaults + persistence round-trip. + * Mirrors `useSettings-reviewAi.test.ts`'s structure. + */ +import { describe, it, expect, beforeEach } from "vitest"; +import { defaultAppSettings, loadSettings, saveSettings } from "../useSettings"; + +describe("Commit Review settings (v3.7.0)", () => { + beforeEach(() => { + localStorage.clear(); + }); + + it("defaults to opt-in-off: commitReviewEnabled false, commitReviewAutoReReview true", () => { + expect(defaultAppSettings.commitReviewEnabled).toBe(false); + expect(defaultAppSettings.commitReviewAutoReReview).toBe(true); + }); + + it("round-trips through save/load", () => { + saveSettings({ + ...defaultAppSettings, + commitReviewEnabled: true, + commitReviewAutoReReview: false, + }); + const loaded = loadSettings(); + expect(loaded.commitReviewEnabled).toBe(true); + expect(loaded.commitReviewAutoReReview).toBe(false); + }); + + it("loadSettings backfills defaults for a stored payload missing the new fields", () => { + const legacy = { ...defaultAppSettings } as Record; + delete legacy.commitReviewEnabled; + delete legacy.commitReviewAutoReReview; + localStorage.setItem("gitwand-settings", JSON.stringify(legacy)); + const loaded = loadSettings(); + expect(loaded.commitReviewEnabled).toBe(false); + expect(loaded.commitReviewAutoReReview).toBe(true); + }); +}); diff --git a/apps/desktop/src/composables/useCommitReview.ts b/apps/desktop/src/composables/useCommitReview.ts new file mode 100644 index 00000000..bcf10102 --- /dev/null +++ b/apps/desktop/src/composables/useCommitReview.ts @@ -0,0 +1,225 @@ +/** + * useCommitReview.ts + * + * Task 1a (v3.7.0) — staged-diff AI review orchestrator ("Commit Review", + * roadmap bullet 1). Opt-in, off by default: with `commitReviewEnabled` + * false this composable performs zero IPC and zero LLM calls. + * + * Reuses the exact same engine as the v3.6.0 PR pre-review pass + * (`usePrPreReview.analyzeFile`, `scope: "commit"` — Task 0), the same + * sequential/abortable/visibility-gated queue (`usePrReviewQueue`), and the + * same confidence-threshold + top-N cap + dismissal filter + * (`usePrFindingFilter`) — see AGENTS.md/plan decision D3: no new + * commit-specific threshold/cap settings, the existing `reviewAi*` knobs + * are reused. + * + * The staged diff is fetched via the existing `gitExec(["diff", "--cached", + * "--no-color"])` primitive (precedent: `useCommitMessage.ts`) — no new + * Tauri command. + */ +import { ref, computed, type ComputedRef, type Ref } from "vue"; +import { gitExec } from "../utils/backend"; +import { indexDiffFiles, parseFileDiff } from "../utils/unifiedDiff"; +import { usePrPreReview, type ReviewFinding } from "./usePrPreReview"; +import { usePrReviewQueue } from "./usePrReviewQueue"; +import { filterFindings, normalizeFindingClass } from "./usePrFindingFilter"; +import { useSettings } from "./useSettings"; +import { useAIProvider } from "./useAIProvider"; +import { useI18n } from "./useI18n"; + +/** + * Hard cap on the number of staged files sent through the review pass — a + * huge staged tree (e.g. a vendored dependency bump) must not fan out + * hundreds of LLM calls. Plan decision D12 flags this number as a guess + * worth validating against a real large staged change before merge. + */ +export const COMMIT_REVIEW_MAX_FILES = 40; + +/** + * Hard cap on the total staged-diff bytes sent through the review pass, + * applied in file-slice order (earlier files in the diff win). Same D12 + * caveat as `COMMIT_REVIEW_MAX_FILES`. + */ +export const COMMIT_REVIEW_MAX_BYTES = 400_000; + +export interface UseCommitReviewOptions { + /** + * Reserved for parity with `useSecretsScanner`'s `debounceMs` — that + * composable's `scan()` is debounced because it's driven by a staged-set + * *watcher*. `run()` here is invoked directly by an explicit user click + * (Task 1b's "Review staged changes" button); Task 3's one-shot + * re-review-after-fix is the only future watcher-driven call site, and + * that phase is out of scope for this PR. Not applied within this PR. + * Default 400. + */ + debounceMs?: number; +} + +export interface CommitReviewResult { + /** Filtered (threshold + cap + session-dismissed) — what the UI renders. */ + findings: Ref; + /** Raw findings from the last completed/in-flight run, unfiltered. */ + rawFindings: Ref; + running: Ref; + progress: ComputedRef<{ done: number; total: number }>; + lastError: Ref; + /** Per-file finding count for the staged list (Task 2's sidebar chips). */ + findingsByFile: ComputedRef>; + /** Deterministic i18n one-liner composed from the findings (decision D2 — + * no second LLM call for the summary). */ + summary: ComputedRef; + /** True once the staged diff was truncated by the file-count or byte cap. */ + truncated: Ref; + run: (cwd: string, locale: string) => Promise; + /** Abort any in-flight run and clear all state (repo switch, post-commit, + * or a staged-set change invalidating a stale review). */ + reset: () => void; + /** Session-only, class-normalized dismissal — mirrors the PR pre-review + * dismissal contract. */ + dismiss: (id: string) => void; + /** Wake the queue after `document.hidden` flips back. The queue itself + * owns the pause/resume state; call this from the host's existing + * `visibilitychange` handler — never add a second listener. */ + resume: () => void; +} + +export function useCommitReview(_opts: UseCommitReviewOptions = {}): CommitReviewResult { + const { settings } = useSettings(); + const { t } = useI18n(); + const ai = useAIProvider(); + const { analyzeFile } = usePrPreReview(); + const queue = usePrReviewQueue(); + + const rawFindings = ref([]); + const lastError = ref(null); + const dismissedClasses = ref>(new Set()); + const truncated = ref(false); + + let abortController: AbortController | null = null; + + const findings = computed(() => + filterFindings(rawFindings.value, { + threshold: settings.value.reviewAiConfidenceThreshold, + cap: settings.value.reviewAiMaxFindings, + dismissed: dismissedClasses.value, + }), + ); + + const progress = computed(() => ({ done: queue.done.value, total: queue.total.value })); + const running = queue.running; + + const findingsByFile = computed>(() => { + const counts: Record = {}; + for (const f of findings.value) counts[f.path] = (counts[f.path] ?? 0) + 1; + return counts; + }); + + const summary = computed(() => { + const list = findings.value; + if (list.length === 0) return t("commitReview.summaryClean"); + const risk = list.filter((f) => f.severity === "risk").length; + const suggestion = list.filter((f) => f.severity === "suggestion").length; + const nit = list.filter((f) => f.severity === "nit").length; + const files = new Set(list.map((f) => f.path)).size; + return t("commitReview.summaryCounts", risk, suggestion, nit, files); + }); + + function stop() { + abortController?.abort(); + abortController = null; + } + + function reset() { + stop(); + rawFindings.value = []; + lastError.value = null; + truncated.value = false; + } + + async function run(cwd: string, locale: string): Promise { + // A new run always supersedes whatever is in flight — abort first so a + // stale in-flight run can never paint findings after this one starts. + stop(); + rawFindings.value = []; + lastError.value = null; + truncated.value = false; + + // Opt-in feature: zero IPC, zero LLM call when disabled/unavailable. + if (!settings.value.commitReviewEnabled || !ai.isAvailable.value || !cwd) return; + + const controller = new AbortController(); + abortController = controller; + + try { + const res = await gitExec(cwd, ["diff", "--cached", "--no-color"]); + if (controller.signal.aborted) return; + + if (res.exitCode !== 0) { + lastError.value = (res.stderr ?? "").trim() || t("errors.commitReviewFailed"); + return; + } + // A clean index is not an error — no findings, no toast. + if (!res.stdout.trim()) return; + + let slices = indexDiffFiles(res.stdout); + if (slices.length > COMMIT_REVIEW_MAX_FILES) { + slices = slices.slice(0, COMMIT_REVIEW_MAX_FILES); + truncated.value = true; + } + + let budget = COMMIT_REVIEW_MAX_BYTES; + const bounded: typeof slices = []; + for (const slice of slices) { + if (budget <= 0) { + truncated.value = true; + break; + } + bounded.push(slice); + budget -= slice.raw.length; + } + + const files = bounded.map((s) => parseFileDiff(s.raw)).filter((f) => f.hunks.length > 0); + if (!files.length) return; + + await queue.run( + files, + (file) => analyzeFile(file, { cwd, locale, otherDiffFiles: files, scope: "commit" }), + { + onFinding: (finding) => { + if (controller.signal.aborted) return; + rawFindings.value = [...rawFindings.value, finding]; + }, + signal: controller.signal, + }, + ); + } catch (err) { + if (!controller.signal.aborted) { + lastError.value = err instanceof Error ? err.message : String(err); + } + } finally { + if (abortController === controller) abortController = null; + } + } + + function dismiss(id: string) { + const finding = rawFindings.value.find((f) => f.id === id); + if (!finding) return; + const cls = normalizeFindingClass(finding); + dismissedClasses.value = new Set([...dismissedClasses.value, cls]); + } + + return { + findings, + rawFindings, + running, + progress, + lastError, + findingsByFile, + summary, + truncated, + run, + reset, + dismiss, + resume: queue.resume, + }; +} diff --git a/apps/desktop/src/composables/useSettings.ts b/apps/desktop/src/composables/useSettings.ts index 040821ed..66454b50 100644 --- a/apps/desktop/src/composables/useSettings.ts +++ b/apps/desktop/src/composables/useSettings.ts @@ -389,6 +389,22 @@ export interface AppSettings { * high-entropy detection pass. 0 disables it. Default: 4.0. */ secretsEntropyThreshold: number; + + // ── v3.7.0 Commit Review ────────────────────────────────── + /** + * Master switch for the local, opt-in AI review pass over the staged diff + * (Commit Review). Off by default — it spends tokens on every "Review + * staged changes" click. A repo's `.gitwandrc` `commitReview.enabled` can + * override this per-repo in either direction (task 6, out of scope here). + * Default: false. + */ + commitReviewEnabled: boolean; + /** + * After a "Fix with agent" handoff (task 3, out of scope here), arm exactly + * one automatic re-review on the next staging change. Only consulted once + * that phase ships. Default: true. + */ + commitReviewAutoReReview: boolean; } export type TerminalMode = "floating" | "fullscreen" | "bottom"; @@ -481,6 +497,8 @@ export const defaultAppSettings: AppSettings = { filesHideOnNav: true, secretsScannerEnabled: true, secretsEntropyThreshold: 4.0, + commitReviewEnabled: false, + commitReviewAutoReReview: true, }; const SETTINGS_KEY = "gitwand-settings"; diff --git a/apps/desktop/src/locales/en.ts b/apps/desktop/src/locales/en.ts index eee00e39..f64cce99 100644 --- a/apps/desktop/src/locales/en.ts +++ b/apps/desktop/src/locales/en.ts @@ -1244,6 +1244,14 @@ const en = { summary: "PR summary", summaryHint: "Generate a what/why/affected-areas summary on the Info tab.", }, + commitReview: { + title: "Commit Review", + hint: "Local, opt-in AI pass over your staged changes, run from the commit area.", + enabled: "Review staged changes", + enabledHint: "Show a \"Review staged changes\" button in the commit area. Off by default, uses the confidence threshold and cap from Review AI above.", + autoReReview: "Auto re-review after fixing with an agent", + autoReReviewHint: "Automatically run one more review the next time you stage changes after handing findings to an agent.", + }, language: "Interface language", languageAuto: "Automatic (system)", commitMessageLang: "Commit message language", @@ -2120,6 +2128,7 @@ const en = { aiResponseInvalidJson: "The AI provider's response could not be interpreted (invalid JSON).", sameBranches: "Head and base branches are identical \u2014 no PR possible.", noChangesToStash: "No local changes to stash \u2014 modify files before generating a message.", + commitReviewFailed: "Commit review failed to read the staged diff.", }, // ─── Tags panel (v1.9) ────────────────────────────────── @@ -2466,6 +2475,31 @@ const en = { ignoreConfirmTitle: "Stop scanning this file for secrets?", ignoreConfirmMessage: "This will stop flagging ANY secret pattern in {0} — permanently, written to .gitwandrc. Continue?", }, + + // ─── Commit Review (v3.7.0) ───────────────────────────── + commitReview: { + reviewButton: "Review staged changes", + reviewButtonRunning: "Reviewing... {0}/{1}", + badgeTooltip: "{0} finding(s) in the staged changes", + modalTitle: "Commit review", + modalSubtitle: "{0} finding(s) in your staged changes", + empty: "No findings", + severityRisk: "Risk", + severitySuggestion: "Suggestion", + severityNit: "Nit", + confidence: "{0}% confidence", + jumpTo: "Jump to", + dismiss: "Dismiss", + close: "Close", + truncatedNotice: "Reviewed the first {0} file(s) of your staged changes.", + summaryClean: "No issues found in your staged changes.", + summaryCounts: "{0} risk(s), {1} suggestion(s), {2} nit(s) across {3} file(s).", + fileCountTooltip: "{0} review finding(s) in this file", + navNext: "Next finding", + navPrev: "Previous finding", + navHelp: "Commit review shortcuts", + navEmpty: "No findings to navigate", + }, } as const; // Widen literal string types to plain `string` for locale compatibility diff --git a/apps/desktop/src/locales/es.ts b/apps/desktop/src/locales/es.ts index 97f97d58..4f5c6326 100644 --- a/apps/desktop/src/locales/es.ts +++ b/apps/desktop/src/locales/es.ts @@ -1219,6 +1219,14 @@ const es: Locale = { summary: "Resumen de PR", summaryHint: "Generar un resumen qué/por qué/áreas afectadas en la pestaña Info.", }, + commitReview: { + title: "Revisión de commit", + hint: "Pase de IA local y opt-in sobre tus cambios en stage, ejecutado desde el área de commit.", + enabled: "Revisar cambios en stage", + enabledHint: "Muestra un botón «Revisar cambios en stage» en el área de commit. Desactivado por defecto, reutiliza el umbral de confianza y el máximo de Review AI de arriba.", + autoReReview: "Nueva revisión automática tras corregir con un agente", + autoReReviewHint: "Ejecutar automáticamente una revisión más la próxima vez que hagas stage de cambios después de enviar los hallazgos a un agente.", + }, language: "Idioma de la interfaz", languageAuto: "Automático (sistema)", commitMessageLang: "Idioma de los mensajes de commit", @@ -2079,6 +2087,7 @@ const es: Locale = { aiResponseInvalidJson: "La respuesta del proveedor IA no pudo interpretarse (JSON inválido).", sameBranches: "La rama fuente y la rama destino son idénticas — ninguna PR posible.", noChangesToStash: "Sin cambios locales para el stash — modifica archivos antes de generar un mensaje.", + commitReviewFailed: "La revisión de commit no pudo leer los cambios en stage.", }, // ─── Tags panel (v1.9) tags: { @@ -2424,6 +2433,31 @@ const es: Locale = { ignoreConfirmTitle: "¿Dejar de escanear este archivo en busca de secretos?", ignoreConfirmMessage: "Esto dejará de marcar CUALQUIER patrón de secreto en {0} — de forma permanente, escrito en .gitwandrc. ¿Continuar?", }, + + // ─── Commit Review (v3.7.0) ───────────────────────────── + commitReview: { + reviewButton: "Revisar cambios en stage", + reviewButtonRunning: "Revisando... {0}/{1}", + badgeTooltip: "{0} hallazgo(s) en los cambios en stage", + modalTitle: "Revisión de commit", + modalSubtitle: "{0} hallazgo(s) en tus cambios en stage", + empty: "Sin hallazgos", + severityRisk: "Riesgo", + severitySuggestion: "Sugerencia", + severityNit: "Detalle", + confidence: "{0}% de confianza", + jumpTo: "Ir a", + dismiss: "Descartar", + close: "Cerrar", + truncatedNotice: "Se revisaron los primeros {0} archivo(s) de tus cambios en stage.", + summaryClean: "No se encontraron problemas en tus cambios en stage.", + summaryCounts: "{0} riesgo(s), {1} sugerencia(s), {2} detalle(s) en {3} archivo(s).", + fileCountTooltip: "{0} hallazgo(s) de revisión en este archivo", + navNext: "Siguiente hallazgo", + navPrev: "Hallazgo anterior", + navHelp: "Atajos de la revisión de commit", + navEmpty: "Sin hallazgos para recorrer", + }, }; export default es; diff --git a/apps/desktop/src/locales/fr.ts b/apps/desktop/src/locales/fr.ts index a16d76ee..672006a9 100644 --- a/apps/desktop/src/locales/fr.ts +++ b/apps/desktop/src/locales/fr.ts @@ -1228,6 +1228,14 @@ const fr: Locale = { summary: "Résumé de PR", summaryHint: "Générer un résumé quoi/pourquoi/zones affectées dans l'onglet Info.", }, + commitReview: { + title: "Revue de commit", + hint: "Passe IA locale et opt-in sur vos changements indexés, lancée depuis la zone de commit.", + enabled: "Passer en revue les changements indexés", + enabledHint: "Afficher un bouton « Passer en revue les changements indexés » dans la zone de commit. Désactivé par défaut, réutilise le seuil de confiance et le plafond de Review AI ci-dessus.", + autoReReview: "Nouvelle revue automatique après une correction par un agent", + autoReReviewHint: "Lancer automatiquement une revue supplémentaire la prochaine fois que vous indexez des changements après avoir transmis les constats à un agent.", + }, language: "Langue de l\u2019interface", languageAuto: "Automatique (syst\u00e8me)", commitMessageLang: "Langue des messages de commit", @@ -2088,6 +2096,7 @@ const fr: Locale = { aiResponseInvalidJson: "La r\u00e9ponse du provider IA n'a pas pu \u00eatre interpr\u00e9t\u00e9e (JSON invalide).", sameBranches: "La branche source et la branche cible sont identiques \u2014 aucune PR possible.", noChangesToStash: "Aucun changement local \u00e0 stasher \u2014 modifie des fichiers avant de g\u00e9n\u00e9rer un message.", + commitReviewFailed: "La revue de commit n'a pas pu lire les changements index\u00e9s.", }, // ─── Tags panel (v1.9) tags: { @@ -2434,6 +2443,31 @@ const fr: Locale = { ignoreConfirmTitle: "Arrêter d'analyser ce fichier pour des secrets ?", ignoreConfirmMessage: "Ceci arrêtera de signaler N'IMPORTE QUEL motif de secret dans {0} — de façon permanente, écrit dans .gitwandrc. Continuer ?", }, + + // ─── Commit Review (v3.7.0) ───────────────────────────── + commitReview: { + reviewButton: "Passer en revue les changements indexés", + reviewButtonRunning: "Revue en cours... {0}/{1}", + badgeTooltip: "{0} constat(s) dans les changements indexés", + modalTitle: "Revue de commit", + modalSubtitle: "{0} constat(s) dans vos changements indexés", + empty: "Aucun constat", + severityRisk: "Risque", + severitySuggestion: "Suggestion", + severityNit: "Détail", + confidence: "{0}% de confiance", + jumpTo: "Aller à", + dismiss: "Ignorer", + close: "Fermer", + truncatedNotice: "Revue effectuée sur les {0} premier(s) fichier(s) de vos changements indexés.", + summaryClean: "Aucun problème trouvé dans vos changements indexés.", + summaryCounts: "{0} risque(s), {1} suggestion(s), {2} détail(s) sur {3} fichier(s).", + fileCountTooltip: "{0} constat(s) de revue dans ce fichier", + navNext: "Constat suivant", + navPrev: "Constat précédent", + navHelp: "Raccourcis de la revue de commit", + navEmpty: "Aucun constat à parcourir", + }, }; export default fr; diff --git a/apps/desktop/src/locales/pt-BR.ts b/apps/desktop/src/locales/pt-BR.ts index 633b102e..50c50f35 100644 --- a/apps/desktop/src/locales/pt-BR.ts +++ b/apps/desktop/src/locales/pt-BR.ts @@ -1220,6 +1220,14 @@ const ptBR: Locale = { summary: "Resumo de PR", summaryHint: "Gerar um resumo o quê/por quê/áreas afetadas na aba Info.", }, + commitReview: { + title: "Revisão de commit", + hint: "Passagem de IA local e opt-in sobre suas mudanças em stage, executada na área de commit.", + enabled: "Revisar mudanças em stage", + enabledHint: "Mostra um botão \"Revisar mudanças em stage\" na área de commit. Desativado por padrão, reutiliza o limite de confiança e o máximo do Review AI acima.", + autoReReview: "Nova revisão automática após corrigir com um agente", + autoReReviewHint: "Executar automaticamente mais uma revisão na próxima vez que você colocar mudanças em stage após enviar os achados a um agente.", + }, language: "Idioma da interface", languageAuto: "Automático (sistema)", commitMessageLang: "Idioma das mensagens de commit", @@ -2079,6 +2087,7 @@ const ptBR: Locale = { aiResponseInvalidJson: "A resposta do provedor de IA não pôde ser interpretada (JSON inválido).", sameBranches: "A branch de origem e destino são idênticas — nenhuma PR possível.", noChangesToStash: "Sem alterações locais para stash — modifique arquivos antes de gerar uma mensagem.", + commitReviewFailed: "A revisão de commit não conseguiu ler as mudanças em stage.", }, // ─── Tags panel (v1.9) tags: { @@ -2424,6 +2433,31 @@ const ptBR: Locale = { ignoreConfirmTitle: "Parar de verificar segredos neste arquivo?", ignoreConfirmMessage: "Isso vai parar de sinalizar QUALQUER padrão de segredo em {0} — permanentemente, gravado em .gitwandrc. Continuar?", }, + + // ─── Commit Review (v3.7.0) ───────────────────────────── + commitReview: { + reviewButton: "Revisar mudanças em stage", + reviewButtonRunning: "Revisando... {0}/{1}", + badgeTooltip: "{0} achado(s) nas mudanças em stage", + modalTitle: "Revisão de commit", + modalSubtitle: "{0} achado(s) nas suas mudanças em stage", + empty: "Nenhum achado", + severityRisk: "Risco", + severitySuggestion: "Sugestão", + severityNit: "Detalhe", + confidence: "{0}% de confiança", + jumpTo: "Ir para", + dismiss: "Dispensar", + close: "Fechar", + truncatedNotice: "Revisados os primeiros {0} arquivo(s) das suas mudanças em stage.", + summaryClean: "Nenhum problema encontrado nas suas mudanças em stage.", + summaryCounts: "{0} risco(s), {1} sugestão(ões), {2} detalhe(s) em {3} arquivo(s).", + fileCountTooltip: "{0} achado(s) de revisão neste arquivo", + navNext: "Próximo achado", + navPrev: "Achado anterior", + navHelp: "Atalhos da revisão de commit", + navEmpty: "Nenhum achado para navegar", + }, }; export default ptBR; diff --git a/apps/desktop/src/locales/zh-CN.ts b/apps/desktop/src/locales/zh-CN.ts index da9fbd5f..a064eee8 100644 --- a/apps/desktop/src/locales/zh-CN.ts +++ b/apps/desktop/src/locales/zh-CN.ts @@ -1278,6 +1278,14 @@ const zhCN: Locale = { summary: "PR 摘要", summaryHint: "在信息标签页生成内容/原因/受影响范围摘要。", }, + commitReview: { + title: "提交审查", + hint: "本地、可选启用的 AI 分析，针对暂存的更改，从提交区域运行。", + enabled: "审查暂存的更改", + enabledHint: "在提交区域显示“审查暂存的更改”按钮。默认关闭，复用上方 Review AI 的置信度阈值和上限。", + autoReReview: "使用代理修复后自动重新审查", + autoReReviewHint: "将发现项交给代理后，下次暂存更改时自动再运行一次审查。", + }, language: "界面语言", languageAuto: "自动（跟随系统）", commitMessageLang: "提交信息语言", @@ -2064,6 +2072,7 @@ const zhCN: Locale = { aiResponseInvalidJson: "无法解析 AI 服务商的响应（JSON 无效）。", sameBranches: "源分支与目标分支相同 — 无法创建 PR。", noChangesToStash: "没有可存储的本地更改 — 请先修改文件再生成消息。", + commitReviewFailed: "提交审查未能读取暂存的更改。", }, // ─── Tags panel (v1.9) tags: { @@ -2409,6 +2418,31 @@ const zhCN: Locale = { ignoreConfirmTitle: "停止扫描此文件中的密钥？", ignoreConfirmMessage: "这将永久停止标记 {0} 中的任何密钥规则——并写入 .gitwandrc。是否继续？", }, + + // ─── 提交审查 (v3.7.0) ───────────────────────────── + commitReview: { + reviewButton: "审查暂存的更改", + reviewButtonRunning: "正在审查... {0}/{1}", + badgeTooltip: "暂存的更改中有 {0} 项发现", + modalTitle: "提交审查", + modalSubtitle: "您暂存的更改中有 {0} 项发现", + empty: "没有发现项", + severityRisk: "风险", + severitySuggestion: "建议", + severityNit: "细节", + confidence: "置信度 {0}%", + jumpTo: "跳转到", + dismiss: "忽略", + close: "关闭", + truncatedNotice: "已审查您暂存更改中的前 {0} 个文件。", + summaryClean: "您暂存的更改中未发现问题。", + summaryCounts: "{3} 个文件中共有 {0} 个风险、{1} 个建议、{2} 个细节问题。", + fileCountTooltip: "此文件中有 {0} 项审查发现", + navNext: "下一个发现项", + navPrev: "上一个发现项", + navHelp: "提交审查快捷键", + navEmpty: "没有可导航的发现项", + }, }; export default zhCN; From 2903a58a62bdb5225659ea39b24f275da15d26ad Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Thu, 13 Aug 2026 17:03:41 +0200 Subject: [PATCH 03/46] feat(commit-review): review staged changes from the commit area with inline findings Wire the Commit Review engine into the UI: - RepoSidebar.vue: a "Review staged changes" button (shown only when the opt-in setting is on and something is staged) with a spinner and done/total progress, plus a findings badge that opens the summary modal. Also adds a per-file finding count chip in both the flat-list and tree-layout file renderers (RepoSidebar has two independent renderers for the file list; both needed the chip). - DiffViewer.vue: a new optional `findings` prop. Inline mode renders one finding row per anchored line (severity badge, confidence, title, detail, Dismiss), reusing the existing prAnnotations grouping so LEFT/RIGHT findings on the same line number never merge and an orphan finding (line not in the diff) renders nothing instead of throwing. Side-by-side mode gets a gutter severity marker only, no card (decision D4 - full SBS cards are a follow-up). Findings render via plain-text interpolation only, never v-html. Exposes scrollToFinding(line, side) for Task 2's navigation. - CommitReviewModal.vue: new component (modelled on SecretsFindingsModal.vue) showing the deterministic summary line and a severity-then-confidence-sorted finding list with Jump to/Dismiss per finding and an empty state. - App.vue: instantiates useCommitReview, wires the button/badge/modal, passes per-file findings to DiffViewer (index-scoped, so never painted on an unstaged diff), and extends the existing staged-set watch to reset() the review (never auto-run) whenever the staged set changes. Add commitReview.* i18n keys (all 5 locales) for the new UI strings. --- apps/desktop/src/App.vue | 63 ++++- .../src/components/CommitReviewModal.vue | 186 +++++++++++++ apps/desktop/src/components/DiffViewer.vue | 263 +++++++++++++++--- apps/desktop/src/components/RepoSidebar.vue | 123 ++++++++ .../__tests__/CommitReviewModal.test.ts | 96 +++++++ .../__tests__/DiffViewer-findings.test.ts | 134 +++++++++ apps/desktop/src/locales/en.ts | 2 +- apps/desktop/src/locales/es.ts | 2 +- apps/desktop/src/locales/fr.ts | 2 +- apps/desktop/src/locales/pt-BR.ts | 2 +- apps/desktop/src/locales/zh-CN.ts | 2 +- 11 files changed, 825 insertions(+), 50 deletions(-) create mode 100644 apps/desktop/src/components/CommitReviewModal.vue create mode 100644 apps/desktop/src/components/__tests__/CommitReviewModal.test.ts create mode 100644 apps/desktop/src/components/__tests__/DiffViewer-findings.test.ts diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index f24546e6..cc7b0ebe 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -70,6 +70,7 @@ const EditCommitOverlay = defineAsyncComponent(() => import("./components/EditCo const SplitCommitModal = defineAsyncComponent(() => import("./components/SplitCommitModal.vue")); const BranchDirtySwitchModal = defineAsyncComponent(() => import("./components/BranchDirtySwitchModal.vue")); const SecretsFindingsModal = defineAsyncComponent(() => import("./components/SecretsFindingsModal.vue")); +const CommitReviewModal = defineAsyncComponent(() => import("./components/CommitReviewModal.vue")); import { useStashMessage } from "./composables/useStashMessage"; import { useAIProvider } from "./composables/useAIProvider"; import { usePrPanel, PR_PANEL_KEY } from "./composables/usePrPanel"; @@ -95,6 +96,7 @@ import { useScheduler } from "./composables/useScheduler"; import { useRepoPoller } from "./composables/useRepoPoller"; import { useLaunchpadPoller } from "./composables/useLaunchpadPoller"; import { useSecretsScanner } from "./composables/useSecretsScanner"; +import { useCommitReview } from "./composables/useCommitReview"; import { useLaunchpadPrs } from "./composables/useLaunchpadPrs"; import { diffLaunchpad, isBotAuthor, type LaunchpadEvent } from "./composables/useLaunchpadNotifications"; import { osNotify } from "./composables/useOsNotification"; @@ -116,7 +118,7 @@ import { import { gitStash, gitStashPop, gitStashList, openInEditor, setGitConfig, gitDiscard, gitAddToGitignore, gitDeleteBranch, gitDeleteTag, gitDeleteRemoteTag, gitRemoteInfo, gitUnpushedTags, gitPushTags, gitMergeBase, gitResetToCommit, gitCommitSubmoduleChanges, gitSubmoduleCheckUpdates, scratchWorktreeCreate, scratchWorktreeDiscard, scratchWorktreeMergeBack, gitWorktreeList, gitWorktreeRemove, type CommitSubmoduleChange } from "./utils/backend"; import { useCommitActions } from "./composables/useCommitActions"; -const { t } = useI18n(); +const { t, locale } = useI18n(); const { settings, refreshSettings } = useSettings(); const { saveMemory } = useResolutionMemory(); @@ -126,6 +128,10 @@ const showSecretsModal = ref(false); /** Trailers of the last commit attempt — reused by the findings modal's "Commit anyway" so it * doesn't need its own copy of RepoSidebar's Signed-off-by trailer logic. */ let lastAttemptedCommitTrailers = ""; + +// v3.7.0 — Commit Review (local, opt-in, off by default; see useCommitReview.ts). +const commitReview = useCommitReview(); +const showCommitReviewModal = ref(false); // `useNetworkStatus` covers `navigator.onLine` — kept around because // `useScheduler` already consumes it and we don't want to retire that path // in this commit. `useConnectivity` (F1) adds a real probe-based signal @@ -267,6 +273,17 @@ const { worktreeBranches, } = useGitRepo({ confirm: askConfirm }); +// v3.7.0 — Commit Review: template ref to the mounted DiffViewer (its +// `scrollToFinding` is called by Task 2's nav composable), and the findings +// for the file currently displayed. Findings are index-scoped (reviewed the +// staged diff, not the working tree), so never paint them on an unstaged diff. +const diffViewerRef = ref(null); +const findingsForSelectedFile = computed(() => + repoSelectedFileStaged.value && repoSelectedFile.value + ? commitReview.findings.value.filter((f) => f.path === repoSelectedFile.value) + : [], +); + // Monorepo scope (v2.21.0) — restore persisted scope on repo open. const { loadScope } = useWorkspaceScope(); @@ -1016,6 +1033,11 @@ const repoSidebarProps = computed(() => ({ visibleFileIdx: historyVisibleFileIdx.value, gitUser: currentGitUser.value, secretFindingsCount: secretsScanner.activeFindings.value.length, + commitReviewEnabled: settings.value.commitReviewEnabled, + commitReviewRunning: commitReview.running.value, + commitReviewFindingsCount: commitReview.findings.value.length, + commitReviewProgress: commitReview.progress.value, + reviewFindingsByFile: commitReview.findingsByFile.value, })); /** @@ -1065,6 +1087,20 @@ function onSecretsCommitAnyway() { void doCommit(lastAttemptedCommitTrailers); } +/** v3.7.0 — "Jump to" in the findings modal: select the finding's file + * (always staged — findings are index-scoped) and scroll the diff to it. */ +function onJumpToCommitReviewFinding(id: string) { + const finding = commitReview.findings.value.find((f) => f.id === id); + if (!finding) return; + if (repoSelectedFile.value !== finding.path || !repoSelectedFileStaged.value) { + repoSelectFile(finding.path, true); + } + showCommitReviewModal.value = false; + void nextTick(() => { + diffViewerRef.value?.scrollToFinding?.(finding.line, finding.side); + }); +} + const repoSidebarListeners = { select: (path: string, staged: boolean) => onRepoFileSelect(path, staged), changeView: (mode: ViewMode) => onViewModeChange(mode), @@ -1088,6 +1124,8 @@ const repoSidebarListeners = { deleteBranch: (name: string, hasLocal: boolean, hasRemote: boolean, remoteName?: string) => handleDeleteBranchRequest(name, hasLocal, hasRemote, remoteName), openSecrets: () => { showSecretsModal.value = true; }, + reviewStaged: () => { void commitReview.run(repoFolderPath.value ?? "", locale.value); }, + openCommitReview: () => { showCommitReviewModal.value = true; }, }; // Trigger a (debounced) secrets scan whenever the staged set changes, or when a repo is @@ -1100,6 +1138,12 @@ watch( } else { secretsScanner.findings.value = []; } + // v3.7.0 — Commit Review: a staged-set/repo change invalidates whatever + // findings are on screen (the diff they reviewed no longer exists). + // Never auto-run here — the pass only runs on the explicit "Review + // staged changes" click (decision D5; the one-shot re-review after a + // "Fix with agent" handoff is Task 3, out of scope for this PR). + commitReview.reset(); }, { immediate: true }, ); @@ -3349,10 +3393,12 @@ onUnmounted(() => { - + @select-dir-file="(path) => repoSelectFile(path, false)" + @dismiss-finding="(id) => commitReview.dismiss(id)" />
{ @close="showSecretsModal = false" /> + + +
+ + {{ reviewFindingsByFile![file.path] }}
+ + {{ reviewFindingsByFile![row.path] }}
+ + +
- + {{ reviewFindingsByFile![file.path] }} @@ -1377,9 +1380,11 @@ function formatActivityDate(dateStr: string): string {
+ this branch is easy to miss, hence the explicit callout. + Gated on section === 'staged' — see the flat-list branch + above for why. --> {{ reviewFindingsByFile![row.path] }} diff --git a/apps/desktop/src/components/SettingsPanel.vue b/apps/desktop/src/components/SettingsPanel.vue index cf2578a6..bde358fc 100644 --- a/apps/desktop/src/components/SettingsPanel.vue +++ b/apps/desktop/src/components/SettingsPanel.vue @@ -2770,15 +2770,12 @@ function deleteReleaseNoteTemplate(id: string) { {{ t('settings.commitReview.enabledHint') }}
-
- - {{ t('settings.commitReview.autoReReviewHint') }} -
+ From ebb9f178f11f37e53ad5af52197020fe502be203 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Thu, 13 Aug 2026 19:05:22 +0200 Subject: [PATCH 10/46] docs: add v3.7.0 commit review implementation plan Records the step-by-step plan behind the Commit Review feature (ROADMAP.md v3.7.0), including the decisions on 3-way PR splitting and warn-only pre-commit hook scope. --- .../plans/2026-08-13-v3.7.0-commit-review.md | 788 ++++++++++++++++++ 1 file changed, 788 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-13-v3.7.0-commit-review.md diff --git a/docs/superpowers/plans/2026-08-13-v3.7.0-commit-review.md b/docs/superpowers/plans/2026-08-13-v3.7.0-commit-review.md new file mode 100644 index 00000000..3c90e5bd --- /dev/null +++ b/docs/superpowers/plans/2026-08-13-v3.7.0-commit-review.md @@ -0,0 +1,788 @@ +# v3.7.0 — Commit Review: micro AI reviews in the Changes panel + +> **For agentic workers:** REQUIRED SUB-SKILL: use `superpowers:subagent-driven-development` +> (recommended) or `superpowers:executing-plans` to implement this plan task-by-task. +> Steps use checkbox (`- [ ]`) syntax for tracking. Work the tasks **in order** — later tasks +> depend on modules introduced earlier. Every task ends with a named conventional commit; the +> branch must stay green after each commit (`cd apps/desktop && pnpm test` + `pnpm -r run test`). + +**Branch:** `feat/v3.7-commit-review` (this worktree). Baseline verified clean: `packages/core` +1056 tests pass, `apps/desktop` 689 tests pass. + +**Roadmap item:** `ROADMAP.md` → "v3.7.0 — Commit Review: micro AI reviews in the Changes panel" +(6 bullets). This plan implements those 6 bullets as tasks 1–6 plus one foundation task (task 0). +There is no separate spec file; the roadmap bullets + this plan are the spec. Every judgment call +this plan had to make is listed in **§Open decisions** at the bottom — read that section before +starting, and do not silently re-decide any of it. + +--- + +## Goal + +Bring the v3.6.0 AI pre-review pipeline to commit time, in the Changes panel, fully local: +one button in the commit area runs an AI pass over the staged diff, findings are anchored +inline in `DiffViewer`, navigable finding-to-finding, pipeable to a CLI agent, tracked across +review→fix→review iterations, and recorded as a `GitWand-Review` commit trailer via an explicit +three-state (ran / vouched / skipped) non-blocking decision. Off by default, per-repo opt-in. + +## What already exists (verified by reading the code, not assumed) + +| Piece | Where | Reusable as-is? | +|---|---|---| +| Multi-hop AI review engine, `GitDiff` in → `ReviewFinding[]` out | `apps/desktop/src/composables/usePrPreReview.ts:255-280` (`analyzeFile`) | **Yes** — already forge-agnostic and PR-agnostic in its signature. Only its *prompt* says "pull request" (`:125`). | +| `ReviewFinding` type (id/path/line/side/severity/confidence/title/detail) | `usePrPreReview.ts:25-36` | Yes, unchanged | +| Defensive JSON-array parse | `usePrPreReview.ts:211` (`parseFindings`) | Yes, unchanged | +| Sequential, abortable, visibility-gated queue | `usePrReviewQueue.ts:23-80` | Yes, unchanged | +| Confidence threshold + top-N cap + dismissal-class filter | `usePrFindingFilter.ts:18-42` | Yes, unchanged | +| Line-anchored annotation model + grouping + worst-severity | `prAnnotations.ts:19-124` (`fromFinding`, `annotationsByLine`, `worstSeverity`) | Yes, unchanged | +| Pure keymap resolver pattern + `isEditableTarget` | `usePrReviewKeymap.ts:27-77` | `isEditableTarget` reused (task 0 extracts it); the action union is PR-specific → new resolver | +| Finding-cursor nav pattern (`N`/`P`, wrap, cross-file) | `usePrReviewNav.ts:95-116` (`jumpToFinding`) | Pattern reused, new composable (different host/rows) | +| Pure unified-diff index + per-file hunk parse | `usePrPanel.ts:94` (`indexDiffFiles`), `:121` (`parseFileDiff`) | Yes — task 0 lifts them into a pure util so a commit-review composable never imports the PR panel | +| Staged diff fetch, no new command needed | `backend.ts:2227` `gitExec(cwd, ["diff","--cached","--no-color"])` — precedent: `useCommitMessage.ts:146-149` | Yes | +| Trailer assembly + append | `RepoSidebar.vue:551-560` (`buildTrailers`) → `emit("commit", trailers)` (`:786`, `:1696`) → `useGitRepo.ts:910-921` appends the block after a blank line | Yes — the `GitWand-Review` trailer needs **zero** Rust/IPC change | +| Non-blocking commit-area badge UX contract | `RepoSidebar.vue:1659-1671` (badge) + `App.vue:1026-1038` (`handleCommitRequest` confirm gate) + `SecretsFindingsModal.vue` | Pattern mirrored exactly | +| Agent PTY launch (`claude`/`codex`/`opencode`/`antigravity`) | `useTerminalSessions.ts:82-148` (`openTab`, `type` → first-class agent) + `App.vue:1551-1597` (`openTerminalTab`) + `terminalWrite` via `sessions.write` | Yes | +| AI-task scratch worktree flow | `App.vue:1681-1715` (`onNewAiTask`/`confirmNewAiTask`) + `useScratchWorktree.ts` + `useAiTasks.ts` | Yes | +| `.gitwandrc` schema + parser + read/write IPC | `packages/core/src/config.ts:219-324` / `:330-487`; `backend.ts:1479` `readGitwandrc`, `:1509` `writeGitwandrc` | Extended in task 6 (new `commitReview` block, same shape as `secrets`) | +| Pre-commit hook install/remove | `HooksPanel.vue:137-188` + `utils/secretsHook.ts` + `backend.ts:2576` `gitHookCreate` | **Collision** — see task 6; the secrets hook owns the whole `pre-commit` file today | + +**Consequence: no new `#[tauri::command]` is required for tasks 0–5.** Verified: staged diff via +`gitExec`, blame via `getGitBlame` (already used by `analyzeFile`), agent launch via `terminalOpen`, +trailers via the existing commit path, `.gitwandrc` via existing read/write, hooks via existing +`gitHookCreate`/`gitHookDelete`/`readFile`. If a task turns out to need one anyway, it also needs a +`dev-server.mjs` route + a `backend.ts` wrapper in the same commit (AGENTS.md), and parity coverage +if deterministic — stop and re-plan rather than sneaking an `invoke()` in. + +## Architecture at a glance + +``` + ┌─ utils/unifiedDiff.ts (pure: indexDiffFiles / parseFileDiff) [task 0] + │ +useCommitReview.ts ───┼─ usePrPreReview.analyzeFile (scope: "commit") [task 0] + (orchestrator) ├─ usePrReviewQueue.run (abort on staged-set change) + ├─ usePrFindingFilter.filterFindings + └─ commitReviewState.ts (pure: coverage, iterations, trailer) [task 4] + +RepoSidebar.vue → "Review staged changes" button + findings badge + per-file counts [tasks 1,2] +DiffViewer.vue → inline finding rows (severity badge + title + detail) [task 1] +CommitReviewModal.vue → summary + finding list + "Fix with agent" + iter/coverage [tasks 1,3,4] +CommitReviewDecisionModal.vue → Review / Vouch / Skip at commit time [task 5] +App.vue → wiring, keydown host, agent handoff, commit interception +``` + +## Global constraints (enforce in every task) + +- **i18n** — every user-visible string gets a key in **all 5** locales: + `apps/desktop/src/locales/en.ts`, `fr.ts`, `es.ts`, `pt-BR.ts`, `zh-CN.ts` (exact filenames — + note `pt-BR`/`zh-CN` are capitalized). `Locale = Widen` (`locales/en.ts` tail) means + **`vue-tsc` fails the build if any locale is missing a key** — so keys land in all 5 files in the + same commit as the UI that uses them. New top-level blocks introduced by this plan: + `commitReview.*`, `settings.commitReview.*`, `hooks.review*`. Never hardcode user-facing text. +- **Settings sync** — any new field goes in **both** `useSettings.ts` (`AppSettings` interface + + `defaultAppSettings`) **and** `SettingsPanel.vue` (local `Settings` interface + its default + object, `:160-163`/`:253-256` show the `reviewAi*` precedent) in the same commit. +- **Versions** — never hand-edit `package.json` / `Cargo.toml` / `tauri.conf.json` versions. + `./scripts/bump-version.sh 3.7.0` is a tag-time action, out of scope for these PRs. +- **`packages/core` stays zero-Node and browser-safe.** The only core change in this plan is the + `.gitwandrc` `commitReview` block in `config.ts` (pure JSON validation, task 6). +- **IPC** — never `invoke()` outside `utils/backend*.ts`. +- **No unconditional interval / no unconditional work.** The whole feature is opt-in and must do + literally zero work (no IPC, no LLM call, no watcher body) when `commitReviewEnabled` is false. + Follow the staged-set watch pattern at `App.vue:1095-1105` — never a `setInterval` + (`apps/desktop/CLAUDE.md` P6.4). +- **No deep watchers** on `repoFiles`/`repoStatus`/`repoDiff`. Watch specific fields + (`repoStats.staged`, `repoFolderPath`) like the secrets scanner does. +- **Sanitize** — findings come from an LLM. Render them as **plain text interpolation + (`{{ }}`)**, never `v-html`. If a future step needs markdown, it must go through + `useSafeHtml.ts` (DOMPurify). +- **Buttons are rounded squares** — `--radius-sm|md|lg`, never a pill (`apps/desktop/src/CLAUDE.md`). +- **`BaseModal` CSS** — never prefix `.bm-btn` with an ancestor selector (AGENTS.md specificity rule). +- **Diff-parsing gotcha** — this plan deliberately adds **no new diff parser**. Context lines are + detected by a leading space; `parseFileDiff` (`usePrPanel.ts:151-159`) already does this + correctly (`line.startsWith(" ") || line === ""`). Task 0 moves that function verbatim. + If you ever find yourself writing `!line.startsWith("\\")`, stop — that is the documented bug. +- **Lazy-load** every new modal via `defineAsyncComponent` in `App.vue` (pattern: `App.vue:72` + `SecretsFindingsModal`). + +## Test conventions + +- Composable/pure-module tests: `apps/desktop/src/composables/__tests__/` and + `apps/desktop/src/utils/__tests__/`, Vitest + jsdom. **Composable tests mock + `../../utils/backend` and `../useAIProvider`** — the established convention, see + `usePrPreReview.test.ts:11-20`. AGENTS.md's "real git repos, never a mocked git layer" rule + targets Rust/integration tests; it does not require spinning a real repo for a jsdom composable + test that mocks the IPC boundary. (Task 6's `.gitwandrc` parser test is pure — no repo needed.) +- Pure functions (coverage math, trailer builder, keymap resolver, hook script builder) get + hand-built inputs and **no mocks**. +- Component tests only where the render logic is the thing under test (pattern: + `components/__tests__/SecretsFindingsModal.test.ts`, `PrInlineDiff.test.ts`). +- **No Rust/parity work in this plan.** `pnpm test:parity` is unaffected (no command added). If + that changes, run it. +- Every fixed bug gets a regression test in the same commit. + +--- + +# PHASE 0 — Foundation + +## Task 0: Make the review engine diff-source-agnostic + +**Roadmap bullet 1, first half:** "Generalize `usePrHunkCritique` from PR hunks to any `GitDiff` +— the same engine as the v3.5.0 pre-review pass, pointed at the index." + +**Reading note that changes the shape of this task:** the roadmap's phrasing is slightly stale. +`usePrHunkCritique.ts` is the *older*, per-hunk, verdict+prose critique used only by +`PrInlineDiff.vue:20`; the engine the roadmap actually wants ("the same engine as the pre-review +pass") is `usePrPreReview.analyzeFile`, and that one **already takes a plain `GitDiff`** — nothing +PR-shaped in its signature (`usePrPreReview.ts:38-49, 262-277`). So "generalize" here means: +(a) stop hardcoding "pull request" in its prompt, and (b) make its pure inputs importable without +dragging in the PR panel. Do **not** rewrite `usePrHunkCritique`, do **not** rename +`usePrPreReview` (a rename churns 8 test files and `useReviewIntelligence` for zero behavioral +gain — noted as decision D1). + +**Files:** +- Create `apps/desktop/src/utils/unifiedDiff.ts` — move `indexDiffFiles` (`usePrPanel.ts:94-115`) + and `parseFileDiff` (`usePrPanel.ts:121-162`) **verbatim** (including the leading-space context + branch), plus `parseUnifiedDiff` (`:168-170`). Pure module, no Vue, no backend import. +- Modify `apps/desktop/src/composables/usePrPanel.ts` — delete the moved bodies, `import` them + from `../utils/unifiedDiff` and **re-export** (`export { indexDiffFiles, parseFileDiff, + parseUnifiedDiff }`) so `useReviewIntelligence.ts:25` and + `__tests__/usePrPanel-lazy-diff.test.ts:21` keep working untouched. +- Create `apps/desktop/src/utils/editableTarget.ts` — move `isEditableTarget` + (`usePrReviewKeymap.ts:27-38`) verbatim; re-export from `usePrReviewKeymap.ts` for back-compat + (its tests import it from there). +- Modify `apps/desktop/src/composables/usePrPreReview.ts`: + - `buildSystemPrompt(locale)` → `buildSystemPrompt(locale, scope: ReviewScope = "pr")` where + `export type ReviewScope = "pr" | "commit"`. Swap only the framing sentence and the + dependency-signal sentence: `"one file of a pull request"` → `"one file of a commit that is + about to be created (the staged changes)"`, and `"other files in this same PR"` → `"other + files staged in this same commit"`. Severity scale, confidence rules, JSON contract, and the + "never invent symbols" rule stay **byte-identical** — the whole point is one engine. + - `AnalyzeFileOptions` gains `scope?: ReviewScope` (default `"pr"`), threaded into + `buildSystemPrompt` at `:269`. +- No behavior change for any existing PR caller (default `"pr"`). + +**Interfaces produced:** +- `utils/unifiedDiff.ts`: `indexDiffFiles(raw): {path, raw}[]`, `parseFileDiff(slice): GitDiff`. +- `usePrPreReview`: `analyzeFile(file, { cwd, locale, otherDiffFiles, scope: "commit" })`. + +- [ ] **Step 1 (test-first):** Create `apps/desktop/src/utils/__tests__/unifiedDiff.test.ts` — + copy the three existing assertions from `__tests__/usePrPanel-lazy-diff.test.ts:50-80` + (3-file split, empty input, blank-context-line classification) against the new module path. + The blank-context assertion is the gotcha guard — it must fail if someone "simplifies" the + context branch. Red (module doesn't exist) → then move the code → green. +- [ ] **Step 2:** Do the move + re-export. Run `cd apps/desktop && pnpm test` — the *existing* + `usePrPanel-lazy-diff.test.ts` must still pass unmodified. That is the regression proof. +- [ ] **Step 3:** Move `isEditableTarget` + re-export; `usePrReviewKeymap.test.ts` must pass + unmodified. +- [ ] **Step 4 (test-first):** Extend `__tests__/usePrPreReview.test.ts` with two cases: + `scope` omitted → system prompt contains "pull request"; `scope: "commit"` → system prompt + contains the staged-commit framing and **not** "pull request". Assert against the captured + `rawPromptMock` first argument (the mock already exists at `:8`). Then implement. +- [ ] **Step 5:** `pnpm test` (desktop) + `pnpm -r run test` green. No i18n, no settings, no Rust. + +**Done when:** `analyzeFile` can be called with a staged-diff `GitDiff` and a commit-scoped +prompt; the two pure diff helpers are importable from `utils/` with no PR coupling; every +pre-existing test passes **unmodified**. + +**Commit:** `refactor(review): make the AI review engine diff-source-agnostic` + +--- + +# PHASE 1 — Review staged changes (roadmap bullet 1) + +## Task 1a: `useCommitReview` orchestrator + +**Files:** +- Create `apps/desktop/src/composables/useCommitReview.ts`: + ```ts + export interface UseCommitReviewOptions { debounceMs?: number } // default 400, mirrors useSecretsScanner + export interface CommitReviewResult { + findings: Ref; // filtered (threshold + cap + dismissed) + rawFindings: Ref; + running: Ref; + progress: ComputedRef<{ done: number; total: number }>; + lastError: Ref; + findingsByFile: ComputedRef>; // per-file count for the staged list (task 2) + summary: ComputedRef; // deterministic i18n one-liner (decision D2) + run: (cwd: string, locale: string) => Promise; + reset: () => void; // abort + clear (repo switch, post-commit) + dismiss: (id: string) => void; // session-only, class-normalized + } + ``` + Internals: + 1. `gitExec(cwd, ["diff","--cached","--no-color"])`; non-zero exit or empty stdout → `reset()` + and return (no findings, no error toast — a clean index is not an error). + 2. `indexDiffFiles(stdout)` → `parseFileDiff` per slice → drop files with `hunks.length === 0`. + Cap the file count at `COMMIT_REVIEW_MAX_FILES = 40` and the total staged-diff size at + ~400 KB (slice-order truncation), exposing `truncated: Ref` so the UI can say + "reviewed the first N files" — a 500-file staged tree must not fan out 500 LLM calls. + 3. `usePrReviewQueue().run(files, (f) => analyzeFile(f, { cwd, locale, otherDiffFiles: files, + scope: "commit" }), { onFinding, signal })` — one `AbortController` per run, stored at module + scope of the composable instance; a new `run()` or `reset()` aborts the previous one. + 4. `findings` = `filterFindings(rawFindings, { threshold: + settings.reviewAiConfidenceThreshold, cap: settings.reviewAiMaxFindings, dismissed })` — + reuse the *existing* review-AI settings rather than adding two near-duplicate fields + (decision D3). + 5. Guards: return immediately if `!settings.commitReviewEnabled`, `!ai.isAvailable.value`, or + `!cwd`. Never throw out of `run()` — set `lastError` (mirrors `useSecretsScanner:129-133`, + "a scan failure must never disrupt the commit flow"). + 6. Visibility: the queue already pauses on `document.hidden`; expose `resume()` and have + `App.vue` call it from an existing `visibilitychange` handler — **do not** add a second + listener (`usePrReviewQueue.ts:9-13`). +- Modify `apps/desktop/src/composables/useSettings.ts` — add to `AppSettings` + `defaultAppSettings`: + - `commitReviewEnabled: boolean` (default **false** — opt-in, it spends tokens). + - `commitReviewAutoReReview: boolean` (default **true**; only consulted after a "Fix with agent" + handoff, task 3). +- Modify `apps/desktop/src/components/SettingsPanel.vue` — same two fields in the local `Settings` + interface (`:160-163` neighborhood) + defaults (`:253-256`), and a UI block in the **AI tab** + right under the existing `settings.reviewAi` group (`:2701-2744`): a checkbox for + `commitReviewEnabled` + hint, and (shown only when enabled) the `commitReviewAutoReReview` + checkbox. Reuse `sp-group`/`sp-hint`/`updateSetting` exactly as the neighbors do. +- i18n (all 5 locales): `settings.commitReview.{title,hint,enabled,enabledHint,autoReReview, + autoReReviewHint}`, `commitReview.{summaryClean,summaryCounts,truncatedNotice}`, + `errors.commitReviewFailed`. + +- [ ] **Step 1 (test-first):** `apps/desktop/src/composables/__tests__/useCommitReview.test.ts`, + mocking `../../utils/backend` (`gitExec`, `getGitBlame`) and `../useAIProvider` + (`isAvailable`, `rawPrompt`) per `usePrPreReview.test.ts:11-20`. Cases: + 1. disabled setting → zero `gitExec` calls, zero `rawPrompt` calls, `findings` empty; + 2. AI unavailable → same; + 3. two staged files, model returns one finding each → 2 findings, `progress` reaches 2/2, + `findingsByFile` = `{a:1, b:1}`; + 4. `gitExec` exit code 1 → `findings` empty, `lastError` set, no throw; + 5. empty staged diff → no LLM call; + 6. a second `run()` while the first is in flight aborts the first (no stale findings painted); + 7. `reset()` clears findings and aborts; + 8. below-threshold finding is filtered out of `findings` but present in `rawFindings`; + 9. file-count cap: 45 staged files → at most 40 `rawPrompt` calls and `truncated === true`. +- [ ] **Step 2:** Implement `useCommitReview.ts` until green. +- [ ] **Step 3:** Add the two settings fields to **both** files + the AI-tab UI. Add the i18n keys + to all 5 locales; `pnpm build` (vue-tsc) is the check that none is missing. +- [ ] **Step 4:** Extend `__tests__/useSettings-reviewAi.test.ts` (or add + `useSettings-commitReview.test.ts`) asserting both defaults and that a persisted partial + settings object still hydrates them (the `{...defaultAppSettings, ...JSON.parse(raw)}` path, + `useSettings.ts:502`). + +**Done when:** with `commitReviewEnabled` on and a provider configured, `run()` produces filtered +findings over the staged diff; with it off, the composable performs no IPC and no LLM call. + +**Commit:** `feat(commit-review): add staged-diff AI review engine + opt-in setting` + +## Task 1b: Commit-area button + inline findings in the diff + +**Files:** +- Modify `apps/desktop/src/components/RepoSidebar.vue`: + - New props (next to `secretFindingsCount`, `:61`): `commitReviewEnabled?: boolean`, + `commitReviewRunning?: boolean`, `commitReviewFindingsCount?: number`, + `commitReviewProgress?: { done: number; total: number }`. + - New emits (next to `openSecrets`, `:95`): `reviewStaged: []`, `openCommitReview: []`. + - In `.commit-actions` (`:1658-1707`), **before** the commit button: a "Review staged changes" + button, shown only when `commitReviewEnabled && repoStats.staged > 0`, disabled while + `commitReviewRunning`, showing the existing `.commit-spinner` SVG + `done/total` while + running (copy the `commit-ai-btn` loading pattern at `:1466-1478`); and a findings badge + (same markup/skin as `.commit-secrets-badge`, `:1659-1671`, different icon/colour) shown when + `commitReviewFindingsCount > 0`, emitting `openCommitReview`. +- Modify `apps/desktop/src/components/DiffViewer.vue`: + - New optional prop `findings?: ReviewFinding[]` (default `[]`) — the findings for the file + currently displayed. Import the type from `../composables/usePrPreReview`. + - `const findingsByLine = computed(() => annotationsByLine(props.findings.map(fromFinding)))` + — reuse `prAnnotations.ts:62,81`; **do not** write a second grouping function. + - Inline mode only (`:604-633`): after each ``, when + `findingsByLine.get(\`${side}:${lineNo}\`)` is non-empty, render an extra + `` with a `colspan` cell containing, per finding: a severity + badge (`risk`/`suggestion`/`nit` → danger/warning/muted CSS vars), the confidence as + `{{ f.confidence }}%`, the `title`, the `detail`, and a "Dismiss" button emitting + `dismiss-finding`. Plain-text interpolation only — no `v-html`. + - Side-by-side mode (`:667-698`): a gutter/severity marker on the affected line only, no card + (decision D4 — full SBS finding cards are a follow-up). + - `defineExpose({ scrollToFinding })` — `scrollToFinding(line, side)` finds the rendered row + (`hunkEls`, `:177-191` for the coarse hunk scroll; then `querySelector` the + `[data-line-key]` attribute you add to each `tr`) and `scrollIntoView({block:"center"})`. + Needed by task 2; add it here so the DOM contract lands with the markup. + - New emit `dismiss-finding: [id: string]`. +- Create `apps/desktop/src/components/CommitReviewModal.vue` (` + + + + diff --git a/apps/desktop/src/components/__tests__/CommitReviewDecisionModal.test.ts b/apps/desktop/src/components/__tests__/CommitReviewDecisionModal.test.ts new file mode 100644 index 00000000..701d6882 --- /dev/null +++ b/apps/desktop/src/components/__tests__/CommitReviewDecisionModal.test.ts @@ -0,0 +1,89 @@ +/** + * Task 5 (v3.7.0) — `CommitReviewDecisionModal`: Review now / Vouch + * personally / Skip, at commit time. Mounted with native `createApp`, + * mirroring `CommitReviewModal.test.ts`. No custom keyboard shortcuts are + * added here — `BaseModal` already maps Escape/backdrop to `close`, which + * this component treats as "cancel the commit" (decision D8). + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { createApp, type App } from "vue"; +import CommitReviewDecisionModal from "../CommitReviewDecisionModal.vue"; + +let app: App | null = null; +let container: HTMLElement; + +function mount(props: Record = {}) { + container = document.createElement("div"); + document.body.appendChild(container); + app = createApp(CommitReviewDecisionModal, { + findingsCount: 2, + iterations: 1, + coverage: 60, + ...props, + }); + app.mount(container); +} + +beforeEach(() => { + localStorage.clear(); +}); + +afterEach(() => { + app?.unmount(); + app = null; + container?.remove(); +}); + +describe("CommitReviewDecisionModal", () => { + it("renders the current findings/iteration/coverage context", () => { + mount({ findingsCount: 3, iterations: 2, coverage: 75 }); + // BaseModal teleports its content to , so assert against + // document.body rather than the (now-empty) mount container. + expect(document.body.textContent).toContain("3"); + expect(document.body.textContent).toContain("2"); + expect(document.body.textContent).toContain("75"); + }); + + it("emits review-now when Review now is clicked", () => { + const onReviewNow = vi.fn(); + mount({ onReviewNow }); + document.querySelector(".crdm-review-now")!.click(); + expect(onReviewNow).toHaveBeenCalled(); + }); + + it("emits vouch when Vouch personally is clicked", () => { + const onVouch = vi.fn(); + mount({ onVouch }); + document.querySelector(".crdm-vouch")!.click(); + expect(onVouch).toHaveBeenCalled(); + }); + + it("emits skip when Skip is clicked", () => { + const onSkip = vi.fn(); + mount({ onSkip }); + document.querySelector(".crdm-skip")!.click(); + expect(onSkip).toHaveBeenCalled(); + }); + + it("emits close (not skip, not vouch) when the footer Cancel is clicked", () => { + const onClose = vi.fn(); + const onSkip = vi.fn(); + const onVouch = vi.fn(); + mount({ onClose, onSkip, onVouch }); + document.querySelector(".crdm-cancel")!.click(); + expect(onClose).toHaveBeenCalled(); + expect(onSkip).not.toHaveBeenCalled(); + expect(onVouch).not.toHaveBeenCalled(); + }); + + it("the three actions emit distinct events — no click accidentally triggers two", () => { + const onReviewNow = vi.fn(); + const onVouch = vi.fn(); + const onSkip = vi.fn(); + mount({ onReviewNow, onVouch, onSkip }); + document.querySelector(".crdm-vouch")!.click(); + expect(onVouch).toHaveBeenCalledTimes(1); + expect(onReviewNow).not.toHaveBeenCalled(); + expect(onSkip).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts index 55d972dc..b64045ed 100644 --- a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts +++ b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts @@ -193,3 +193,105 @@ describe("recordReview / getState / coverageFor / clear", () => { expect(mod.getState("/repo").iterations).toBe(1); }); }); + +// ── Task 5 (v3.7.0) — GitWand-Review trailer + commit gate ──────────────── +describe("buildReviewTrailer", () => { + it("builds the exact ran/iter/coverage shape", () => { + expect(mod.buildReviewTrailer("ran", 2, 87)).toBe("GitWand-Review: ran (iter:2, coverage:87%)"); + }); + + it("builds vouched and skipped the same way when iter > 0", () => { + expect(mod.buildReviewTrailer("vouched", 1, 50)).toBe("GitWand-Review: vouched (iter:1, coverage:50%)"); + expect(mod.buildReviewTrailer("skipped", 3, 10)).toBe("GitWand-Review: skipped (iter:3, coverage:10%)"); + }); + + it("omits the parenthetical entirely when iter is 0 (decision D9)", () => { + expect(mod.buildReviewTrailer("skipped", 0, 0)).toBe("GitWand-Review: skipped"); + expect(mod.buildReviewTrailer("vouched", 0, 42)).toBe("GitWand-Review: vouched"); + }); + + it("clamps coverage to 0-100", () => { + expect(mod.buildReviewTrailer("ran", 1, 150)).toBe("GitWand-Review: ran (iter:1, coverage:100%)"); + expect(mod.buildReviewTrailer("ran", 1, -20)).toBe("GitWand-Review: ran (iter:1, coverage:0%)"); + }); + + it("floors a negative iter at 0 (which also drops the parenthetical)", () => { + expect(mod.buildReviewTrailer("ran", -5, 80)).toBe("GitWand-Review: ran"); + }); + + it("has no trailing newline", () => { + expect(mod.buildReviewTrailer("ran", 1, 100).endsWith("\n")).toBe(false); + }); + + it("the key is exactly 'GitWand-Review' when parsed by the first colon (git interpret-trailers semantics)", () => { + const trailer = mod.buildReviewTrailer("ran", 2, 87); + const firstColon = trailer.indexOf(":"); + const key = trailer.slice(0, firstColon).trim(); + expect(key).toBe("GitWand-Review"); + }); +}); + +describe("resolveCommitReviewGate", () => { + it("proceeds straight to commit when the feature is disabled", () => { + expect(mod.resolveCommitReviewGate({ enabled: false, staged: 3, decision: null, iterations: 0 })).toBe("proceed"); + }); + + it("proceeds when nothing is staged", () => { + expect(mod.resolveCommitReviewGate({ enabled: true, staged: 0, decision: null, iterations: 0 })).toBe("proceed"); + }); + + it("prompts when enabled, staged, no decision yet, and no review has run", () => { + expect(mod.resolveCommitReviewGate({ enabled: true, staged: 3, decision: null, iterations: 0 })).toBe("prompt"); + }); + + it("proceeds without re-prompting once a decision is already recorded", () => { + expect(mod.resolveCommitReviewGate({ enabled: true, staged: 3, decision: "vouched", iterations: 0 })).toBe("proceed"); + }); + + it("proceeds without prompting when a review already ran this cycle, even with no explicit decision", () => { + // "Review staged changes" was clicked before ever hitting commit — don't + // re-ask, the review already happened. + expect(mod.resolveCommitReviewGate({ enabled: true, staged: 3, decision: null, iterations: 1 })).toBe("proceed"); + }); +}); + +describe("appendReviewTrailer", () => { + it("appends the GitWand-Review line AFTER the existing trailer block (Signed-off-by, Reviewed-by, ...)", () => { + const existing = "Signed-off-by: A \nReviewed-by: B "; + const review = "GitWand-Review: ran (iter:1, coverage:100%)"; + const result = mod.appendReviewTrailer(existing, review); + const lines = result.split("\n"); + expect(lines).toEqual([ + "Signed-off-by: A ", + "Reviewed-by: B ", + "GitWand-Review: ran (iter:1, coverage:100%)", + ]); + }); + + it("returns just the review trailer when there are no other trailers", () => { + expect(mod.appendReviewTrailer("", "GitWand-Review: skipped")).toBe("GitWand-Review: skipped"); + }); + + it("returns the existing trailers unchanged when there's no review trailer to append", () => { + expect(mod.appendReviewTrailer("Signed-off-by: A ", "")).toBe("Signed-off-by: A "); + }); + + it("returns an empty string when both sides are empty", () => { + expect(mod.appendReviewTrailer("", "")).toBe(""); + }); +}); + +describe("effectiveReviewDecision", () => { + it("returns the explicit decision when one is set", () => { + expect(mod.effectiveReviewDecision("skipped", 0)).toBe("skipped"); + expect(mod.effectiveReviewDecision("vouched", 2)).toBe("vouched"); + }); + + it("defaults to 'ran' when no explicit decision but a review already happened", () => { + expect(mod.effectiveReviewDecision(null, 1)).toBe("ran"); + }); + + it("stays null when no decision and no review happened (the gate should have prompted)", () => { + expect(mod.effectiveReviewDecision(null, 0)).toBeNull(); + }); +}); diff --git a/apps/desktop/src/composables/commitReviewState.ts b/apps/desktop/src/composables/commitReviewState.ts index e5f92501b41b13535049259e7bce8002a6fc73eb..930129b76be4879d63c6dd4caeffcdfcdd0e3c8f 100644 GIT binary patch delta 4041 zcmai1(QX^Z6@BXi+vg@P2Cj)1=BngMZ4o4)ssxH12SMGau+%z06JUqCLvpO;%yws% z6k!YNr+z?{Z|F-NlE3M@zoh5R%q}U}X#hc@mNR$moO{l>cYiqjulVT)`eJZ+NN>J> zjUT!Y_L4rNH?O~^FO_>PbTN4**Gg{4)k2lBrr%L+Rx9NuPOT&rGFMg^O^cf!-+Ry^ z5(oR+H&~>xBX^_>`Um0l2{h5z^z$$>&bG{QH9!5LqlU62Jjt#;*>YMC2cVQ4Q^Rmo!L z(d@H+{Be>F2FTOYj@-7A6aaqQA$dZ>TIeCYqT$*!`H~~xa~JJ+FgQFo7|;QI-l($h zIZ+mh)ppg&x)fv8QaWFXN=|uOT-z<35>Df*pay>!V;`y@7IOOWM-P%LrRPhjDdTT5 zQkITrBshrA?2vP!jdDxU2G?GORT=Whbe zEre5RkW!S5BvD#JMQtjd8?o~wnH#RTk~%(mmm0X*P;-n{-#0tJX|C@!=E;en% zlv^%|yn@US5_N6&6`N60b((wf)wA;p)V73DS=-q{IS!*V3R^*klbEW_$Y=z!kP*a* z<%|MkVI5`DENJ5`U>=5AZeB#HOux{vzBmU5TsF(Kvjbx6CH5oFqBEyX#RD(z|x9lpcYc zF-~n7U5q*^Cn1)a(qFtX~2fYayQ|IQpif;mu^{hjpkm6 z6veDWP6yr(IqC{!^>H|`j{x>pM#Ji8bXryE8jiDsnXr9eKQ}%ttwnnX8lTV;DNt_7 zJ#I|kzBcewWQM~QaIu!{QIs1t3I^LFOrvfUuwozz=_AsFPItg;aIPq8G{&x9v1#^( z?TvRYWMT9}2Tn~e9_*sEuzYs>J`ftoUW}3VU0}(||CElhgnNVT0TP&AjnUfOiQ5*J36}~y z#p+{U?LF5fAT}3G2`dDep)aqMpiE-w$d9;ngzO&2?wxV*0QDI&d7~`)Ks=R|0MmMZ zVNJR2$<6q&Xhup`jhlM&k(#MgOlf9JDTQV^hl*oJ@@B4UcCarkxCjl_NRT@>Cxi^cyn0Vr9$^%Qha#lQ|%7# z+dJ5ccf{zH-(DRZjL&53yz#`z=K_lA4@F@h<_1l&qjJ}_@-`Gd78H+juh6WP?<_)w z{=P4zsi#9fQuZg}=8!Q6=-b{(6qo^^%vkWi5}28$R)Ai&B!Vdn-f>%NBWz{X411M@ zE01Z$=Hyg~*%`BFBxmE6Lky&#mYAW(B>QJ)jDE(!I4HGnGRcP7lm+Ysb54ki^LYyJ zT7{$W(&3(KgaI}jGUi}jhrDVY4GEjzN&A+BCxJ2E9uC8Rdo#ZG>()!; z4Lgve<2~4XTW)c!mksl=FVT*PFe%?MyRVpt@zHG-(q~cNd>RDAjV|&HZVkf^K;0Ri zv;M`%Pkpde#_8!P#x4KAwBe;=&s1im$uBWU`7vm-M2Xn2vS+d~=Wy$ps8c`K#F561 zlB~B_)URlAS@;tF(hQl>7gw(3w=fhOnfZxsqp9MaGH Date: Mon, 17 Aug 2026 11:09:58 +0200 Subject: [PATCH 14/46] test(commit-review): use fake timers for the debounce test to remove flakiness The real-timer version of the debounce/one-shot re-review test had only a few ms of margin between its waits and the debounce window, and failed once under load during full-suite verification. Switches it to fake timers (pattern: useSecretsScanner.test.ts), scoped to just this one test. disabled --- .../__tests__/useCommitReview.test.ts | 39 ++++++++++++------- 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/apps/desktop/src/composables/__tests__/useCommitReview.test.ts b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts index 42829532..a8bd5775 100644 --- a/apps/desktop/src/composables/__tests__/useCommitReview.test.ts +++ b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts @@ -374,20 +374,31 @@ describe("useCommitReview", () => { // quick succession must never double-fire the one-shot re-review, even // when the debounce window overlaps two staged-set events. it("debounces rapid repeated staged-set changes into a single armed re-review", async () => { - enableCommitReview({ commitReviewAutoReReview: true }); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); - rawPromptMock.mockResolvedValue("[]"); - - const review = useCommitReview({ debounceMs: 20 }); - review.armReReview(); - review.onStagedSetChanged("/repo", "en", 1); - // A second staged-set event arrives before the debounce window elapses - // — it must reset the timer, not queue a second run. - await new Promise((resolve) => setTimeout(resolve, 5)); - review.onStagedSetChanged("/repo", "en", 1); - await new Promise((resolve) => setTimeout(resolve, 40)); - - expect(gitExecMock).toHaveBeenCalledTimes(1); + // Fake timers (pattern: useSecretsScanner.test.ts) rather than real + // setTimeout waits — a real-timer version of this test was flaky under + // load (the debounce window and the waits around it are only a few ms + // apart), and fake timers make the sequencing deterministic instead of + // load-dependent. Scoped to just this test; every other test in this + // file keeps using real timers. + vi.useFakeTimers(); + try { + enableCommitReview({ commitReviewAutoReReview: true }); + gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview({ debounceMs: 20 }); + review.armReReview(); + review.onStagedSetChanged("/repo", "en", 1); + // A second staged-set event arrives before the debounce window + // elapses — it must reset the timer, not queue a second run. + await vi.advanceTimersByTimeAsync(5); + review.onStagedSetChanged("/repo", "en", 1); + await vi.advanceTimersByTimeAsync(25); + + expect(gitExecMock).toHaveBeenCalledTimes(1); + } finally { + vi.useRealTimers(); + } }); }); From 8d8ab557100e1c0952f0f440b5a41bb5c5b5da58 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Mon, 17 Aug 2026 12:21:15 +0200 Subject: [PATCH 15/46] fix(commit-review): remove a raw NUL byte that made commitReviewState.ts binary hashLineKey's template literal had a literal, embedded NUL byte instead of the \0 escape sequence, which made the whole file register as binary to git (git diff showed "Binary files differ", GitHub would render it as "Binary file not shown", and git blame/merge/diff were broken on it going forward). Replaces the raw byte with the \0 escape sequence in source, so the runtime hash values are byte-identical (verified: every existing hashLineKey test still passes unmodified) while the file itself is plain UTF-8 text. Adds a regression guard that reads the source file's own bytes and fails if a raw NUL character ever reappears. disabled --- .../__tests__/commitReviewState.test.ts | 23 ++++++++++++++++++ .../src/composables/commitReviewState.ts | Bin 12503 -> 12504 bytes 2 files changed, 23 insertions(+) diff --git a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts index b64045ed..1ccabad0 100644 --- a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts +++ b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts @@ -5,6 +5,9 @@ * no mocks; the store itself is exercised against real (jsdom) localStorage. */ import { describe, it, expect, beforeEach } from "vitest"; +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; import type { GitDiff } from "../../utils/backend"; let mod: typeof import("../commitReviewState"); @@ -15,6 +18,26 @@ beforeEach(async () => { mod._resetCommitReviewStateForTesting(); }); +/** + * Regression guard: this module's source once had a raw, literal NUL byte + * embedded in a template literal (instead of the `\0` escape sequence), + * which made the whole file register as binary to git — GitHub rendered it + * as "Binary file not shown", and git blame/diff broke on it going forward. + * A NUL byte is never valid in this source file; this test fails loudly if + * one silently reappears, in this file or any sibling `.ts` source that + * might copy the same pattern. + */ +describe("source file encoding", () => { + it("commitReviewState.ts contains no raw NUL byte", () => { + // `node:path` join (not `new URL(relative, import.meta.url)`) — jsdom's + // shimmed global `URL` isn't accepted by `fileURLToPath`'s scheme check. + const testDir = dirname(fileURLToPath(import.meta.url)); + const path = join(testDir, "..", "commitReviewState.ts"); + const raw = readFileSync(path); + expect(raw.includes(0)).toBe(false); + }); +}); + function diffWithAddedLines(path: string, added: string[]): GitDiff { return { path, diff --git a/apps/desktop/src/composables/commitReviewState.ts b/apps/desktop/src/composables/commitReviewState.ts index 930129b76be4879d63c6dd4caeffcdfcdd0e3c8f..eef3b4dde29c219c08bafbabf3dd0cac1bd30bcd 100644 GIT binary patch delta 15 Wcmcbfcq4H`B@a`K!R9KSSQP*`O$F}& delta 14 VcmcbScs+4LB@ZLR=4zf;6#y{*1!DjJ From cf0efa9e5d7cfb0e076c8b31f6624d4f9040eceb Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Mon, 17 Aug 2026 12:23:57 +0200 Subject: [PATCH 16/46] fix(commit-review): bind the review cycle to HEAD and keep coverage honest between reviews Two related correctness fixes to the iterations/coverage tracking: 1. Coverage no longer asserts 100% right after a plain staged-set change. onStagedSetChanged now recomputes it against the CURRENT staged diff (a plain git diff fetch, no LLM call), guarded by a generation counter so a stale in-flight refresh can never clobber a newer one or a real run(). Previously a brand-new unreviewed file staged after a completed review kept showing coverage:100% until the next explicit review click. 2. iterations is now bound to the repo's HEAD commit, not just the review count. run() resolves and stamps HEAD via a single git rev-parse HEAD call per run, and reconcileIterationsForHead (awaited by the commit gate before it ever reads iterations) resets the count to 0 when HEAD moved since the last recorded review. Without this, a commit made outside the app (amend, terminal commit, any external tool) left a stale iterations count that let the gate skip the decision modal and write a "ran" trailer for a review that never happened against what was actually being committed. Also resets iterations when snapshot pruning empties a repo's snapshot list entirely (aged-out evidence). disabled --- .../__tests__/commitReviewState.test.ts | 120 +++++++- .../__tests__/useCommitReview.test.ts | 260 +++++++++++++++--- .../src/composables/commitReviewState.ts | 85 +++++- .../src/composables/useCommitReview.ts | 108 +++++++- 4 files changed, 522 insertions(+), 51 deletions(-) diff --git a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts index 1ccabad0..66fbc935 100644 --- a/apps/desktop/src/composables/__tests__/commitReviewState.test.ts +++ b/apps/desktop/src/composables/__tests__/commitReviewState.test.ts @@ -12,12 +12,6 @@ import type { GitDiff } from "../../utils/backend"; let mod: typeof import("../commitReviewState"); -beforeEach(async () => { - localStorage.clear(); - mod = await import("../commitReviewState"); - mod._resetCommitReviewStateForTesting(); -}); - /** * Regression guard: this module's source once had a raw, literal NUL byte * embedded in a template literal (instead of the `\0` escape sequence), @@ -38,6 +32,12 @@ describe("source file encoding", () => { }); }); +beforeEach(async () => { + localStorage.clear(); + mod = await import("../commitReviewState"); + mod._resetCommitReviewStateForTesting(); +}); + function diffWithAddedLines(path: string, added: string[]): GitDiff { return { path, @@ -202,6 +202,50 @@ describe("recordReview / getState / coverageFor / clear", () => { expect(mod.reviewedHashesFor("/repo").size).toBe(0); }); + // ── Verifier item #3 — a stale iterations count must never survive past + // the evidence (snapshots) that justified it aging out. Without this, a + // repo can end up with {iterations: 3, snapshots: []} and the next commit + // silently records "ran (iter:3, coverage:0%)" with no review having + // actually happened and no decision modal shown. + it("resets iterations to 0 when pruning empties the snapshot list on load", () => { + const stale = { + repos: { + "/repo": { + iterations: 3, + updatedAt: Date.now() - 10 * 24 * 60 * 60 * 1000, + headHash: "abc123", + snapshots: [ + { iter: 1, ts: Date.now() - 10 * 24 * 60 * 60 * 1000, lineHashes: ["stale-hash"] }, + ], + }, + }, + }; + localStorage.setItem(mod.COMMIT_REVIEW_STATE_STORAGE_KEY, JSON.stringify(stale)); + mod._resetCommitReviewStateForTesting(); + expect(mod.getState("/repo").iterations).toBe(0); + }); + + it("does NOT reset iterations when pruning only removes SOME snapshots, leaving others", () => { + const now = Date.now(); + const state = { + repos: { + "/repo": { + iterations: 2, + updatedAt: now, + headHash: "abc123", + snapshots: [ + { iter: 1, ts: now - 10 * 24 * 60 * 60 * 1000, lineHashes: ["old-hash"] }, // stale, pruned + { iter: 2, ts: now, lineHashes: ["fresh-hash"] }, // still fresh + ], + }, + }, + }; + localStorage.setItem(mod.COMMIT_REVIEW_STATE_STORAGE_KEY, JSON.stringify(state)); + mod._resetCommitReviewStateForTesting(); + expect(mod.getState("/repo").iterations).toBe(2); + expect(mod.reviewedHashesFor("/repo").has("fresh-hash")).toBe(true); + }); + it("clear removes only the target repo's entry", () => { mod.recordReview("/repo-a", [diffWithAddedLines("a.ts", ["x"])]); mod.recordReview("/repo-b", [diffWithAddedLines("b.ts", ["y"])]); @@ -217,6 +261,70 @@ describe("recordReview / getState / coverageFor / clear", () => { }); }); +// ── Verifier item #3 — bind the review cycle to HEAD ─────────────────────── +// `iterations` must not survive a commit made OUTSIDE `proceedToCommit` +// (amend, a terminal commit, any external tool): without this, the count +// leaks across commits and the next in-app commit can silently write a +// `GitWand-Review: ran (iter:N, ...)` trailer with no review having actually +// happened against the diff that's about to be committed. +describe("recordReview HEAD-cycle binding", () => { + it("recordReview stamps the given headHash on the state", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + expect(mod.getState("/repo").headHash).toBe("sha-1"); + }); + + it("recordReview starts a fresh cycle (iterations back to 1) when headHash differs from what's recorded", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + expect(mod.getState("/repo").iterations).toBe(1); + + // Same HEAD — a normal second review pass in the same cycle. + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["y"])], "sha-1"); + expect(mod.getState("/repo").iterations).toBe(2); + + // HEAD changed (a commit happened) — the next review starts a NEW cycle. + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["z"])], "sha-2"); + expect(mod.getState("/repo").iterations).toBe(1); + }); + + it("does not reset the cycle when no headHash is provided (back-compat default)", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["y"])]); // no third arg + expect(mod.getState("/repo").iterations).toBe(2); + }); +}); + +describe("reconcileHead", () => { + it("resets iterations/snapshots to a fresh cycle when the stored headHash differs from the current one", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + expect(mod.getState("/repo").iterations).toBe(1); + + const next = mod.reconcileHead("/repo", "sha-2"); + expect(next.iterations).toBe(0); + expect(next.snapshots).toEqual([]); + expect(next.headHash).toBe("sha-2"); + expect(mod.getState("/repo").iterations).toBe(0); + }); + + it("is a no-op (just stamps headHash) when nothing was recorded yet for this repo", () => { + const next = mod.reconcileHead("/never-reviewed", "sha-1"); + expect(next.iterations).toBe(0); + expect(next.headHash).toBe("sha-1"); + }); + + it("leaves iterations/snapshots untouched when the headHash is unchanged", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + const next = mod.reconcileHead("/repo", "sha-1"); + expect(next.iterations).toBe(1); + }); + + it("leaves state unchanged when the current headHash can't be resolved (empty string)", () => { + mod.recordReview("/repo", [diffWithAddedLines("a.ts", ["x"])], "sha-1"); + const next = mod.reconcileHead("/repo", ""); + expect(next.iterations).toBe(1); + expect(next.headHash).toBe("sha-1"); + }); +}); + // ── Task 5 (v3.7.0) — GitWand-Review trailer + commit gate ──────────────── describe("buildReviewTrailer", () => { it("builds the exact ran/iter/coverage shape", () => { diff --git a/apps/desktop/src/composables/__tests__/useCommitReview.test.ts b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts index a8bd5775..a096eddf 100644 --- a/apps/desktop/src/composables/__tests__/useCommitReview.test.ts +++ b/apps/desktop/src/composables/__tests__/useCommitReview.test.ts @@ -2,6 +2,18 @@ * Task 1a (v3.7.0) — `useCommitReview` orchestrator: staged-diff AI review * engine, opt-in and off by default. Mocks `../../utils/backend` and * `../useAIProvider` per the established convention (`usePrPreReview.test.ts`). + * + * Verifier item #3 (v3.7.0 PR2 fixes) — `run()` now also resolves the + * repo's HEAD commit (`git rev-parse HEAD`) so a completed review is stamped + * against the commit it actually reviewed, letting the commit-time gate + * detect a commit that happened outside the app since the last review. + * `gitExecMock` therefore routes by git subcommand (`args[0]`) instead of a + * single blind response queue: `rev-parse` calls are answered from + * `headHashResponse` (a fixed stub unless a test explicitly changes HEAD to + * simulate an out-of-band commit), and `diff` calls are answered from the + * `diff*` helpers below — completely decoupling the two so existing + * `mockResolvedValueOnce` diff sequencing never has to account for the + * extra call. */ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; @@ -42,6 +54,27 @@ function gitExecOk(stdout: string) { return { stdout, stderr: "", exitCode: 0 }; } +type ExecResult = { stdout: string; stderr: string; exitCode: number }; + +// ── gitExec routing: rev-parse (HEAD) vs diff (staged diff fetch) ──────── +let headHashResponse = "head-hash-stub"; +let diffOnceQueue: ExecResult[] = []; +let diffDefaultResponse: ExecResult | null = null; + +/** Simulates an out-of-band commit (amend, terminal commit, external tool) + * changing HEAD between two `run()` calls. */ +function setHeadHash(hash: string): void { + headHashResponse = hash; +} + +function queueDiffResponseOnce(res: ExecResult): void { + diffOnceQueue.push(res); +} + +function setDiffDefaultResponse(res: ExecResult): void { + diffDefaultResponse = res; +} + function enableCommitReview(overrides: Partial = {}) { const { settings } = useSettings(); settings.value = { ...defaultAppSettings, commitReviewEnabled: true, ...overrides }; @@ -56,6 +89,15 @@ describe("useCommitReview", () => { rawPromptMock.mockReset(); getGitBlameMock.mockReset().mockResolvedValue([]); gitExecMock.mockReset(); + headHashResponse = "head-hash-stub"; + diffOnceQueue = []; + diffDefaultResponse = null; + gitExecMock.mockImplementation(async (_cwd: string, args: string[]): Promise => { + if (args[0] === "rev-parse") return { stdout: headHashResponse, stderr: "", exitCode: 0 }; + if (diffOnceQueue.length) return diffOnceQueue.shift()!; + if (diffDefaultResponse) return diffDefaultResponse; + return { stdout: "", stderr: "", exitCode: 0 }; + }); isAvailableRef.value = true; const { settings } = useSettings(); settings.value = { ...defaultAppSettings }; @@ -91,7 +133,7 @@ describe("useCommitReview", () => { it("reviews two staged files and aggregates findings + progress + per-file counts", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); + setDiffDefaultResponse(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); rawPromptMock .mockResolvedValueOnce('[{"line": 1, "title": "finding a", "confidence": 80}]') .mockResolvedValueOnce('[{"line": 1, "title": "finding b", "confidence": 80}]'); @@ -107,7 +149,7 @@ describe("useCommitReview", () => { it("sets lastError (never throws) and leaves findings empty when gitExec exits non-zero", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue({ stdout: "", stderr: "fatal: not a git repository", exitCode: 128 }); + setDiffDefaultResponse({ stdout: "", stderr: "fatal: not a git repository", exitCode: 128 }); const review = useCommitReview(); let ran: boolean | undefined; @@ -122,7 +164,7 @@ describe("useCommitReview", () => { it("makes no LLM call on an empty staged diff (clean index is not an error), and reports it ran", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk("")); + setDiffDefaultResponse(gitExecOk("")); const review = useCommitReview(); const ran = await review.run("/repo", "en"); @@ -138,9 +180,8 @@ describe("useCommitReview", () => { let resolveFirst!: (v: string) => void; const pending = new Promise((resolve) => { resolveFirst = resolve; }); - gitExecMock - .mockResolvedValueOnce(gitExecOk(diffFor("a.ts"))) - .mockResolvedValueOnce(gitExecOk(diffFor("c.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("a.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("c.ts"))); rawPromptMock .mockImplementationOnce(() => pending) .mockResolvedValueOnce('[{"line": 1, "title": "finding c", "confidence": 80}]'); @@ -164,7 +205,7 @@ describe("useCommitReview", () => { it("reset() clears findings, error, and aborts any run in flight", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue('[{"line": 1, "title": "finding a", "confidence": 80}]'); const review = useCommitReview(); @@ -179,7 +220,7 @@ describe("useCommitReview", () => { it("keeps a below-threshold finding in rawFindings but filters it out of findings", async () => { enableCommitReview({ reviewAiConfidenceThreshold: 60 }); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue('[{"line": 1, "title": "low-confidence finding", "confidence": 20}]'); const review = useCommitReview(); @@ -192,7 +233,7 @@ describe("useCommitReview", () => { it("caps the staged file count at exactly COMMIT_REVIEW_MAX_FILES and marks truncated", async () => { enableCommitReview(); const manyFiles = Array.from({ length: 45 }, (_, i) => diffFor(`f${i}.ts`)).join("\n"); - gitExecMock.mockResolvedValue(gitExecOk(manyFiles)); + setDiffDefaultResponse(gitExecOk(manyFiles)); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview(); @@ -218,7 +259,7 @@ describe("useCommitReview", () => { "-old", hugeAddedLine, ].join("\n"); - gitExecMock.mockResolvedValue(gitExecOk(`${hugeDiff}\n${diffFor("small.ts")}`)); + setDiffDefaultResponse(gitExecOk(`${hugeDiff}\n${diffFor("small.ts")}`)); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview(); @@ -232,7 +273,7 @@ describe("useCommitReview", () => { it("resume() unblocks a run paused on document.hidden and lets it complete (never stays wedged)", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue('[{"line": 1, "title": "finding a", "confidence": 80}]'); setHidden(true); @@ -261,9 +302,8 @@ describe("useCommitReview", () => { let resolveFirst!: (v: string) => void; const pending = new Promise((resolve) => { resolveFirst = resolve; }); - gitExecMock - .mockResolvedValueOnce(gitExecOk(diffFor("a.ts"))) - .mockResolvedValueOnce(gitExecOk(diffFor("c.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("a.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("c.ts"))); rawPromptMock .mockImplementationOnce(() => pending) .mockResolvedValueOnce("[]"); @@ -298,9 +338,16 @@ describe("useCommitReview", () => { // than merely existing as a callable, unwired function (the exact shape of // bug that shipped `resume()` unwired in PR1). describe("armReReview() / onStagedSetChanged() — one-shot re-review", () => { + // Assertions in this block check `rawPromptMock` (the LLM call — i.e. + // "a review pass actually ran") rather than raw `gitExec` counts: + // `onStagedSetChanged` also fires an unconditional coverage-refresh + // `git diff` call on every invocation (verifier item #2), which is + // orthogonal to whether the ARMED re-review itself fires, so a raw + // gitExec count is no longer the right signal for "did a review run". + it("arms exactly one auto re-review that fires on the next staged-set change", async () => { enableCommitReview({ commitReviewAutoReReview: true }); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview({ debounceMs: 0 }); @@ -308,34 +355,37 @@ describe("useCommitReview", () => { review.onStagedSetChanged("/repo", "en", 1); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).toHaveBeenCalledTimes(1); + expect(rawPromptMock).toHaveBeenCalledTimes(1); }); it("a second staged-set change after the arm has fired runs no further review", async () => { enableCommitReview({ commitReviewAutoReReview: true }); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview({ debounceMs: 0 }); review.armReReview(); review.onStagedSetChanged("/repo", "en", 1); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).toHaveBeenCalledTimes(1); + expect(rawPromptMock).toHaveBeenCalledTimes(1); - gitExecMock.mockClear(); + rawPromptMock.mockClear(); review.onStagedSetChanged("/repo", "en", 1); // arm already consumed — no re-run await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).not.toHaveBeenCalled(); + // The coverage refresh still runs (it's unconditional), but no NEW + // review pass (no further LLM call) fires. + expect(rawPromptMock).not.toHaveBeenCalled(); }); it("never arms when commitReviewAutoReReview is disabled", async () => { enableCommitReview({ commitReviewAutoReReview: false }); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); const review = useCommitReview({ debounceMs: 0 }); review.armReReview(); review.onStagedSetChanged("/repo", "en", 1); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).not.toHaveBeenCalled(); + expect(rawPromptMock).not.toHaveBeenCalled(); }); it("does not fire the armed re-review when the next staged set is empty", async () => { @@ -345,17 +395,17 @@ describe("useCommitReview", () => { review.onStagedSetChanged("/repo", "en", 0); // staged count is 0 — nothing to review await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).not.toHaveBeenCalled(); + expect(rawPromptMock).not.toHaveBeenCalled(); // The arm was still consumed by this (empty) staged-set change — a // LATER non-empty staging event must not unexpectedly trigger a run. review.onStagedSetChanged("/repo", "en", 1); await new Promise((resolve) => setTimeout(resolve, 10)); - expect(gitExecMock).not.toHaveBeenCalled(); + expect(rawPromptMock).not.toHaveBeenCalled(); }); it("onStagedSetChanged always resets findings/error even when nothing is armed", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue('[{"line": 1, "title": "finding a", "confidence": 80}]'); const review = useCommitReview({ debounceMs: 0 }); @@ -383,7 +433,7 @@ describe("useCommitReview", () => { vi.useFakeTimers(); try { enableCommitReview({ commitReviewAutoReReview: true }); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview({ debounceMs: 20 }); @@ -395,7 +445,7 @@ describe("useCommitReview", () => { review.onStagedSetChanged("/repo", "en", 1); await vi.advanceTimersByTimeAsync(25); - expect(gitExecMock).toHaveBeenCalledTimes(1); + expect(rawPromptMock).toHaveBeenCalledTimes(1); } finally { vi.useRealTimers(); } @@ -403,10 +453,78 @@ describe("useCommitReview", () => { }); // ── Task 4 (v3.7.0) — iterations & coverage ─────────────────────────── + // ── Verifier item #2 — coverage must never assert 100% when it's actually + // unknown/stale. `onStagedSetChanged` used to hardcode `coverage.value = + // 100` unconditionally, which meant a brand-new unreviewed file staged + // AFTER a completed review kept showing "100% reviewed" (and could get + // written into a `GitWand-Review: ran (iter:N, coverage:100%)` trailer) + // right up until the user clicked "Review staged changes" again. + describe("coverage recompute on staged-set change (no commit involved)", () => { + it("recomputes coverage against the current staged diff instead of asserting 100%", async () => { + enableCommitReview(); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview({ debounceMs: 0 }); + await review.run("/repo", "en"); + expect(review.coverage.value).toBe(100); + + // A brand-new, never-reviewed file gets staged — no commit happened, + // HEAD is unchanged, but the staged diff now includes unreviewed content. + setDiffDefaultResponse(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); + review.onStagedSetChanged("/repo", "en", 2); + // The refresh is a plain diff fetch (no LLM call) — async, wait for it. + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(review.coverage.value).toBeLessThan(100); + }); + + it("is zero IPC when the feature is disabled (coverage stays at the neutral default)", async () => { + const { settings } = useSettings(); + settings.value = { ...defaultAppSettings, commitReviewEnabled: false }; + + const review = useCommitReview({ debounceMs: 0 }); + review.onStagedSetChanged("/repo", "en", 1); + await new Promise((resolve) => setTimeout(resolve, 10)); + + expect(gitExecMock).not.toHaveBeenCalled(); + expect(review.coverage.value).toBe(100); + }); + + it("a stale in-flight coverage refresh never clobbers a newer staged-set change's result", async () => { + enableCommitReview(); + let resolveSlow!: (v: ExecResult) => void; + const slow = new Promise((resolve) => { resolveSlow = resolve; }); + + let diffCallN = 0; + gitExecMock.mockImplementation(async (_cwd: string, args: string[]): Promise => { + if (args[0] === "rev-parse") return { stdout: headHashResponse, stderr: "", exitCode: 0 }; + diffCallN++; + if (diffCallN === 1) return slow; // first staged-set change's refresh — deliberately slow + return gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`); // second — resolves fast + }); + + const review = useCommitReview({ debounceMs: 0 }); + review.onStagedSetChanged("/repo", "en", 1); // fires the SLOW refresh + await new Promise((resolve) => setTimeout(resolve, 5)); // let it start, still pending + review.onStagedSetChanged("/repo", "en", 2); // fires a second, faster refresh + await new Promise((resolve) => setTimeout(resolve, 10)); // let the fast one resolve + + // The newer refresh's result must be in effect: current diff is + // a.ts+b.ts against zero reviewed hashes so far, so coverage is 0. + expect(review.coverage.value).toBe(0); + + // Now let the STALE slow one resolve — it must not clobber the newer value. + resolveSlow(gitExecOk(diffFor("a.ts"))); + await new Promise((resolve) => setTimeout(resolve, 10)); + expect(review.coverage.value).toBe(0); + }); + }); + describe("iterations / coverage", () => { it("a completed run bumps iterations to 1 and coverage to 100", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview(); @@ -418,7 +536,7 @@ describe("useCommitReview", () => { it("staging a new unreviewed line drops coverage below 100 while the next run is in flight, then back to 100 once it completes", async () => { enableCommitReview(); - gitExecMock.mockResolvedValueOnce(gitExecOk(diffFor("a.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValueOnce("[]"); const review = useCommitReview(); @@ -429,7 +547,7 @@ describe("useCommitReview", () => { const pending = new Promise((resolve) => { resolveSecond = resolve; }); // A bigger staged diff now includes an extra file with a brand-new, // never-reviewed line. - gitExecMock.mockResolvedValueOnce(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); + queueDiffResponseOnce(gitExecOk(`${diffFor("a.ts")}\n${diffFor("b.ts")}`)); rawPromptMock.mockImplementationOnce(() => pending).mockResolvedValueOnce("[]"); const secondRun = review.run("/repo", "en"); @@ -447,9 +565,8 @@ describe("useCommitReview", () => { let resolveFirst!: (v: string) => void; const pending = new Promise((resolve) => { resolveFirst = resolve; }); - gitExecMock - .mockResolvedValueOnce(gitExecOk(diffFor("a.ts"))) - .mockResolvedValueOnce(gitExecOk(diffFor("c.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("a.ts"))); + queueDiffResponseOnce(gitExecOk(diffFor("c.ts"))); rawPromptMock .mockImplementationOnce(() => pending) .mockResolvedValueOnce("[]"); @@ -472,7 +589,7 @@ describe("useCommitReview", () => { it("clearReviewState resets iterations/coverage and the persisted store for that repo", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview(); @@ -492,7 +609,7 @@ describe("useCommitReview", () => { it("onStagedSetChanged refreshes iterations from persisted state when switching repos", async () => { enableCommitReview(); - gitExecMock.mockResolvedValue(gitExecOk(diffFor("a.ts"))); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); rawPromptMock.mockResolvedValue("[]"); const review = useCommitReview(); @@ -503,5 +620,80 @@ describe("useCommitReview", () => { review.onStagedSetChanged("/repo-b", "en", 0); expect(review.iterations.value).toBe(0); }); + + // ── Verifier item #3 — bind the review cycle to HEAD ───────────────── + // `run()` must stamp each completed review against the repo's HEAD at + // review time, and `reconcileIterationsForHead` (the commit-time gate's + // real trigger — App.vue's `proceedToCommit` awaits it before deciding + // whether to show the decision modal) must catch a commit that happened + // OUTSIDE the app (amend, terminal commit, external tool) since the last + // recorded review, instead of trusting a stale `iterations` count. + describe("HEAD-cycle binding", () => { + it("run() stamps the review against the current HEAD", async () => { + enableCommitReview(); + setHeadHash("sha-1"); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(1); + + // A second review against the SAME HEAD continues the same cycle. + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(2); + }); + + it("a review after HEAD changed (an out-of-band commit) starts a fresh cycle", async () => { + enableCommitReview(); + setHeadHash("sha-1"); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(1); + + // Simulates a commit made outside the app (amend, terminal, external + // tool) moving HEAD between two reviews. + setHeadHash("sha-2"); + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(1); // fresh cycle, not 2 + }); + + it("reconcileIterationsForHead resets a stale iterations count when HEAD moved since the last review, with no review run in between", async () => { + enableCommitReview(); + setHeadHash("sha-1"); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(1); + + // An out-of-band commit happens; the user never reviews again before + // hitting commit — this is exactly the scenario the commit-time gate + // (App.vue's `proceedToCommit`) must catch by awaiting this call + // BEFORE checking `iterations` (verifier item #3). + setHeadHash("sha-2"); + await review.reconcileIterationsForHead("/repo"); + + expect(review.iterations.value).toBe(0); + }); + + it("reconcileIterationsForHead is a no-op when HEAD hasn't changed", async () => { + enableCommitReview(); + setHeadHash("sha-1"); + setDiffDefaultResponse(gitExecOk(diffFor("a.ts"))); + rawPromptMock.mockResolvedValue("[]"); + + const review = useCommitReview(); + await review.run("/repo", "en"); + expect(review.iterations.value).toBe(1); + + await review.reconcileIterationsForHead("/repo"); + expect(review.iterations.value).toBe(1); + }); + }); }); }); diff --git a/apps/desktop/src/composables/commitReviewState.ts b/apps/desktop/src/composables/commitReviewState.ts index eef3b4dd..b57654c4 100644 --- a/apps/desktop/src/composables/commitReviewState.ts +++ b/apps/desktop/src/composables/commitReviewState.ts @@ -39,6 +39,19 @@ export interface CommitReviewRepoState { iterations: number; snapshots: ReviewedSnapshot[]; updatedAt: number; + /** + * HEAD commit hash the current `iterations`/`snapshots` are valid for. + * Verifier item #3 (v3.7.0 PR2 fixes) — without this, a commit made + * OUTSIDE `proceedToCommit` (an amend, a terminal commit, any external + * tool) leaves `iterations` pointing at a review cycle for a commit that + * no longer exists; the next in-app commit would then silently write + * `GitWand-Review: ran (iter:N, ...)` with no review having actually + * happened against what's about to be committed. `""` means "not yet + * known" (a fresh repo, or state persisted before this field existed) — + * an empty stored value never by itself triggers a reset, only an actual + * MISMATCH between two known hashes does (see `recordReview`/`reconcileHead`). + */ + headHash: string; } interface CommitReviewStateFile { @@ -50,7 +63,7 @@ function emptyFile(): CommitReviewStateFile { } function emptyRepoState(): CommitReviewRepoState { - return { iterations: 0, snapshots: [], updatedAt: 0 }; + return { iterations: 0, snapshots: [], updatedAt: 0, headHash: "" }; } // Strictly-increasing write clock — same rationale as `usePrCache.ts`'s @@ -109,11 +122,21 @@ export function computeCoverage(current: string[], reviewed: Set): numbe return Math.round((100 * hit) / current.length); } +/** + * Drops snapshots older than `MAX_AGE_MS`. Verifier item #3 — if pruning + * empties the snapshot list entirely, every piece of evidence backing + * `iterations` has aged out; the count itself is no longer meaningful and + * must reset to 0 rather than silently surviving with no snapshots left to + * justify it (otherwise a stale `{iterations: 3, snapshots: []}` would let + * the next commit skip the decision modal and write a lying trailer). + * Leaves `iterations` alone when pruning only removed SOME snapshots. + */ function pruneStaleSnapshots(state: CommitReviewRepoState, now: number): CommitReviewRepoState { - return { - ...state, - snapshots: state.snapshots.filter((s) => s && now - s.ts <= MAX_AGE_MS), - }; + const snapshots = state.snapshots.filter((s) => s && now - s.ts <= MAX_AGE_MS); + if (snapshots.length === 0 && state.snapshots.length > 0) { + return { ...state, snapshots, iterations: 0 }; + } + return { ...state, snapshots }; } function loadFromStorage(): CommitReviewStateFile { @@ -129,7 +152,8 @@ function loadFromStorage(): CommitReviewStateFile { const iterations = typeof state.iterations === "number" ? state.iterations : 0; const snapshots = Array.isArray(state.snapshots) ? state.snapshots : []; const updatedAt = typeof state.updatedAt === "number" ? state.updatedAt : now; - repos[cwd] = pruneStaleSnapshots({ iterations, snapshots, updatedAt }, now); + const headHash = typeof state.headHash === "string" ? state.headHash : ""; + repos[cwd] = pruneStaleSnapshots({ iterations, snapshots, updatedAt, headHash }, now); } return { repos }; } catch { @@ -189,21 +213,62 @@ export function coverageFor(cwd: string, files: GitDiff[]): number { * iteration counter and appends a new snapshot of the added-line hashes it * covered. Caps snapshot count (oldest evicted) and per-snapshot hash count * (truncated) so a huge staged tree can't blow the localStorage quota. + * + * `headHash` (verifier item #3, optional — defaults to "" for back-compat) + * is the repo's current HEAD commit at review time. When it's provided and + * differs from whatever was last recorded for this repo, a commit happened + * since the last review — the cycle restarts (iterations back to 1, old + * snapshots dropped) instead of counting this review against a diff that no + * longer applies to the current HEAD. An empty/omitted `headHash` never + * triggers a reset by itself — only an actual mismatch between two KNOWN + * hashes does. */ -export function recordReview(cwd: string, files: GitDiff[]): void { +export function recordReview(cwd: string, files: GitDiff[], headHash: string = ""): void { const key = normaliseCwd(cwd); const existing = getState(cwd); - const iterations = existing.iterations + 1; + const startingFresh = !!headHash && !!existing.headHash && existing.headHash !== headHash; + const base = startingFresh ? emptyRepoState() : existing; + const iterations = base.iterations + 1; let lineHashes = addedLineKeys(files); if (lineHashes.length > MAX_HASHES) lineHashes = lineHashes.slice(0, MAX_HASHES); const snapshot: ReviewedSnapshot = { iter: iterations, ts: monoNow(), lineHashes }; - let snapshots = [...existing.snapshots, snapshot]; + let snapshots = [...base.snapshots, snapshot]; if (snapshots.length > MAX_SNAPSHOTS) snapshots = snapshots.slice(snapshots.length - MAX_SNAPSHOTS); - _file.repos[key] = { iterations, snapshots, updatedAt: Date.now() }; + _file.repos[key] = { + iterations, + snapshots, + updatedAt: Date.now(), + headHash: headHash || existing.headHash, + }; + saveToStorage(_file); +} + +/** + * Reconciles the persisted state for `cwd` against the repo's CURRENT HEAD + * commit hash, without recording a new review pass. Used right before the + * commit-time decision gate checks `iterations` (verifier item #3): if a + * HEAD hash was previously recorded and it differs from `currentHeadHash`, + * a commit happened since the last recorded review (an amend, a terminal + * commit, or anything outside `proceedToCommit`) — the cycle resets to 0 + * iterations / no snapshots so the gate re-prompts instead of trusting a + * stale count. A no-op (just stamps the hash) when there's nothing to + * reconcile against yet, or when `currentHeadHash` can't be resolved + * (empty string — e.g. a brand-new repo with no commits). + */ +export function reconcileHead(cwd: string, currentHeadHash: string): CommitReviewRepoState { + const key = normaliseCwd(cwd); + const existing = getState(cwd); + if (!currentHeadHash || existing.headHash === currentHeadHash) return existing; + + const next: CommitReviewRepoState = existing.headHash + ? { ...emptyRepoState(), headHash: currentHeadHash, updatedAt: Date.now() } + : { ...existing, headHash: currentHeadHash }; + _file.repos[key] = next; saveToStorage(_file); + return next; } /** Drop all recorded state for `cwd` — a new commit starts a new review cycle. */ diff --git a/apps/desktop/src/composables/useCommitReview.ts b/apps/desktop/src/composables/useCommitReview.ts index 2ad762b3..36c31e71 100644 --- a/apps/desktop/src/composables/useCommitReview.ts +++ b/apps/desktop/src/composables/useCommitReview.ts @@ -31,6 +31,7 @@ import { coverageFor, getState as getCommitReviewState, clear as clearCommitReviewState, + reconcileHead, } from "./commitReviewState"; /** @@ -136,6 +137,15 @@ export interface CommitReviewResult { * starts a new review cycle. */ clearReviewState: (cwd: string) => void; + /** + * Verifier item #3 — reconciles `iterations` against the repo's CURRENT + * HEAD, without running a new review. The host (App.vue's + * `proceedToCommit`) MUST await this before checking `iterations` at + * commit time, so a commit made outside the app since the last review + * (amend, terminal commit, any external tool) resets the stale count + * instead of silently letting the gate skip the decision modal. + */ + reconcileIterationsForHead: (cwd: string) => Promise; } export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReviewResult { @@ -182,6 +192,14 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview const reReviewArmed = ref(false); let reReviewTimer: ReturnType | null = null; + // Verifier item #2 — generation counter guarding the async, fire-and- + // forget coverage refresh (`refreshCoverageForCurrentDiff`) against a + // stale response landing AFTER a newer staged-set change (or a real + // `run()`) has already superseded it. Bumped by both `onStagedSetChanged` + // and `run()` — whichever started MOST RECENTLY wins the right to set + // `coverage.value`. + let coverageGeneration = 0; + let abortController: AbortController | null = null; const findings = computed(() => @@ -240,6 +258,75 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview coverage.value = 100; } + /** + * Resolves the repo's current HEAD commit hash — a single cheap, read-only + * `git rev-parse HEAD` call, no different in kind from the existing + * staged-diff fetch. Verifier item #3: this is how a completed review + * gets stamped against the exact commit it reviewed, so the commit-time + * gate (`reconcileIterationsForHead`, called from App.vue's + * `proceedToCommit`) can tell whether a commit happened outside the app + * since the last review. Returns `""` (never throws) on any failure — + * a HEAD resolution failure must never break the review pass itself. + */ + async function resolveHeadHash(cwd: string): Promise { + try { + const res = await gitExec(cwd, ["rev-parse", "HEAD"]); + return res.exitCode === 0 ? res.stdout.trim() : ""; + } catch { + return ""; + } + } + + /** + * Verifier item #2 — recomputes `coverage` against the repo's CURRENT + * staged diff (a plain `git diff --cached` fetch, no LLM call) instead of + * leaving whatever neutral default `reset()` just set. Fire-and-forget + * from `onStagedSetChanged`; `generation` (captured at call time against + * the shared `coverageGeneration` counter) guards against this stale + * response landing after a NEWER staged-set change or a real `run()` has + * already superseded it. Zero IPC when the feature is disabled or `cwd` + * is empty — resolves to the neutral 100 default in that case. + */ + async function refreshCoverageForCurrentDiff(cwd: string, generation: number): Promise { + let next = 100; + if (cwd && settings.value.commitReviewEnabled) { + try { + const res = await gitExec(cwd, ["diff", "--cached", "--no-color"]); + if (res.exitCode === 0 && res.stdout.trim()) { + const files = indexDiffFiles(res.stdout) + .map((s) => parseFileDiff(s.raw)) + .filter((f) => f.hunks.length > 0); + next = coverageFor(cwd, files); + } + } catch { + // A coverage-refresh failure must never disrupt anything else — + // leave `next` at the neutral default. + } + } + if (generation === coverageGeneration) coverage.value = next; + } + + /** + * Verifier item #3 — reconciles `iterations` against the repo's CURRENT + * HEAD without running a new review. Call this before checking + * `iterations` at commit time (`App.vue`'s `proceedToCommit`, right + * before `resolveCommitReviewGate`): if a commit happened outside the app + * (amend, terminal commit, any external tool) since the last recorded + * review, this resets the stale count to 0 so the gate re-prompts instead + * of silently trusting a review that no longer applies to what's about to + * be committed. + */ + async function reconcileIterationsForHead(cwd: string): Promise { + if (!cwd) return; + const headHash = await resolveHeadHash(cwd); + const state = reconcileHead(cwd, headHash); + iterations.value = state.iterations; + // A reset also invalidates whatever "reviewed" baseline `coverage` was + // showing — back to the neutral default until the next `run()` parses + // the actual current staged diff. + if (state.snapshots.length === 0) coverage.value = 100; + } + async function run(cwd: string, locale: string): Promise { // A new run always supersedes whatever is in flight — abort first so a // stale in-flight run can never paint findings after this one starts. @@ -247,6 +334,10 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview rawFindings.value = []; lastError.value = null; truncated.value = false; + // Verifier item #2 — a real run supersedes any in-flight coverage-only + // refresh (`refreshCoverageForCurrentDiff`); its stale response must not + // clobber whatever `coverage.value` this run itself computes below. + coverageGeneration++; // Opt-in feature: zero IPC, zero LLM call when disabled/unavailable. // Never even attempted — callers must not treat this as "ran clean". @@ -260,6 +351,11 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview activeQueue.value = queue; try { + // Verifier item #3 — resolve HEAD once per run so a completed review + // gets stamped against the exact commit it reviewed. + const headHash = await resolveHeadHash(cwd); + if (controller.signal.aborted) return false; + const res = await gitExec(cwd, ["diff", "--cached", "--no-color"]); if (controller.signal.aborted) return false; @@ -313,7 +409,7 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview // iteration. Guarded by the abort check above so a stale run's // eventual completion (verifier issue #6's race) never double-counts // an iteration that a newer run has already superseded. - recordReview(cwd, files); + recordReview(cwd, files, headHash); iterations.value = getCommitReviewState(cwd).iterations; coverage.value = coverageFor(cwd, files); return true; @@ -349,6 +445,15 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview // state for whatever repo is now active. reset(cwd); + // Verifier item #2 — `reset()` just set `coverage` to the neutral 100 + // default, which is a LIE if the current staged diff actually has + // unreviewed content (e.g. a brand-new file staged after the last + // completed review). Recompute it for real against the current staged + // diff — a plain `git diff` fetch, no LLM call, zero IPC when the + // feature is disabled (guarded inside `refreshCoverageForCurrentDiff`). + const myCoverageGeneration = ++coverageGeneration; + void refreshCoverageForCurrentDiff(cwd, myCoverageGeneration); + if (!reReviewArmed.value) return; // Debounced consumption: rapid repeated staged-set changes (stage → fix @@ -386,5 +491,6 @@ export function useCommitReview(opts: UseCommitReviewOptions = {}): CommitReview iterations, coverage, clearReviewState, + reconcileIterationsForHead, }; } From 2e21c72341fd8995fb64549e5461dcc72b2e1b41 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Mon, 17 Aug 2026 12:24:18 +0200 Subject: [PATCH 17/46] fix(commit-review): reconcile HEAD before the commit gate, fix Review now, harden the agent handoff - proceedToCommit awaits commitReview.reconcileIterationsForHead before resolveCommitReviewGate ever reads iterations, so a commit made outside the app since the last review is caught before the gate decision (see the companion commitReviewState/useCommitReview fix). - onCommitReviewDecisionReviewNow ("Review now" in the decision modal) now reuses onReviewStagedClicked directly instead of a weaker duplicate: it only opens the findings modal when a review actually completed without error, and surfaces "no AI provider configured" via repoError when the run never even attempted. Previously it always popped an empty "No findings" modal regardless of outcome. - onCommitReviewFixWithAgent adds a short readiness wait before typing the prompt into a freshly spawned agent PTY, and surfaces scratch-worktree and terminal-session failures instead of silently doing nothing after the review modal has already closed. Manual QA performed against real claude and codex CLIs via the dev-server's node-pty backend (see PR report for the full write-up): once an agent is at its normal ready-to-chat input, writing the whole multi-line prompt as one burst lands as unsent text with no premature submission. A first-run "trust this directory?" onboarding screen (always hit by the scratch- worktree path, since it is always a brand-new directory) is a separate, unresolved risk this readiness wait does not cover, flagged in code comments and the PR report for explicit sign-off before merge. disabled --- apps/desktop/src/App.vue | 94 +++++++++++++++++++++++++++++++++++----- 1 file changed, 83 insertions(+), 11 deletions(-) diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index 5ab73ef8..301db21a 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -1156,8 +1156,17 @@ async function handleCommitRequest(trailers: string) { * function ever sees the staged-changes trailers, so recomputing the review * trailer for the CURRENT decision on every call (including the second pass * after a decision is made) is the only correct ordering. + * + * Verifier item #3 — `reconcileIterationsForHead` is awaited FIRST, before + * the gate ever reads `commitReview.iterations`: a commit made outside the + * app since the last recorded review (an amend, a terminal commit, any + * external tool) must reset that count to 0, not silently let the gate skip + * the decision modal and write a `GitWand-Review: ran` trailer for a review + * that never happened against what's actually about to be committed. */ async function proceedToCommit(trailers: string) { + await commitReview.reconcileIterationsForHead(repoFolderPath.value ?? ""); + const gate = resolveCommitReviewGate({ enabled: settings.value.commitReviewEnabled, staged: repoStats.value.staged, @@ -1189,15 +1198,35 @@ async function proceedToCommit(trailers: string) { } } -/** "Review now" in the decision modal: runs the pass and leaves the commit - * un-issued (decision stays whatever it was — null unless a review already - * happened) so the user can look at findings, then commits again when - * satisfied; `resolveCommitReviewGate` then sees `iterations > 0` and - * proceeds without re-prompting. */ +/** + * "Review now" in the decision modal: runs the pass and leaves the commit + * un-issued (decision stays whatever it was — null unless a review already + * happened) so the user can look at findings, then commits again when + * satisfied; `resolveCommitReviewGate` then sees `iterations > 0` and + * proceeds without re-prompting. + * + * Verifier item #4 — reuses `onReviewStagedClicked`'s exact success/ + * failure/clean-pass branching (calling it directly, not duplicating a + * weaker version) rather than always popping the findings modal regardless + * of outcome. Without this, clicking "Review now" with no AI provider + * configured popped an empty "No findings" modal instead of surfacing the + * real reason nothing happened. A genuine failure is already surfaced + * globally via the `commitReview.lastError` watcher above (`repoError`) — + * the one case that watcher can't cover is "never even attempted" + * (`ran === false`), which this modal only reaches via AI being + * unavailable (the feature and a staged repo are already guaranteed by + * `resolveCommitReviewGate` before this modal ever opens). + */ async function onCommitReviewDecisionReviewNow() { showCommitReviewDecisionModal.value = false; - await commitReview.run(repoFolderPath.value ?? "", locale.value); - showCommitReviewModal.value = true; + const ran = await onReviewStagedClicked(); + if (!ran) { + repoError.value = t("errors.noAiProviderShort"); + return; + } + if (!commitReview.lastError.value) { + showCommitReviewModal.value = true; + } } async function onCommitReviewDecisionVouch() { @@ -1253,12 +1282,17 @@ function onSecretsCommitAnyway() { * superseded by a newer run) with no error and zero findings — otherwise a * clean review is indistinguishable from "didn't run" or "failed" * (verifier issue #5). A failed run is separately surfaced via the - * `commitReview.lastError` watcher above (issue #4). */ -async function onReviewStagedClicked() { + * `commitReview.lastError` watcher above (issue #4). Returns whether the + * run actually attempted — `onCommitReviewDecisionReviewNow` ("Review now" + * in the decision modal) reuses this exact branching directly (verifier + * item #4) instead of duplicating a weaker version that ignored the + * outcome and always popped the findings modal. */ +async function onReviewStagedClicked(): Promise { const ran = await commitReview.run(repoFolderPath.value ?? "", locale.value); if (ran && !commitReview.lastError.value && commitReview.findings.value.length === 0) { showCommitReviewCleanToast(); } + return ran; } /** v3.7.0 — "Jump to" in the findings modal: select the finding's file @@ -1963,6 +1997,30 @@ async function confirmNewAiTask(name: string) { * without submitting avoids racing the agent TUI's boot while keeping "let * an agent edit my files" a deliberate human gesture). Optionally opens the * agent in a scratch worktree first (reuses `createAiTaskScratchWorktree`). + * + * Verifier item #5 — manual QA performed against real `claude` and `codex` + * CLIs (via the dev-server's real `node-pty` backend, the same one + * `pnpm dev:web` uses) confirmed: once an agent has reached its normal + * ready-to-chat input state, writing this whole multi-line, trailing-\n + * prompt as one burst lands as UNSENT multi-line input text (verified for + * `claude` — its bracketed-paste-mode input box shows every line, with no + * submission and no response activity for several seconds after). That + * part of decision D7's assumption holds. + * + * BUT: a brand-new working directory (exactly what "in a scratch worktree" + * always is) makes both `claude` and `codex` show a first-run "trust this + * directory?" onboarding prompt before their normal input box exists, and + * for `codex` a subsequent "update available" prompt can follow. Writing + * newline-bearing input into THOSE screens does not "type unsent text" — + * it drives their Enter-confirms-the-highlighted-option menu navigation. + * In manual testing this went as far as `codex` starting a real `brew + * upgrade --cask codex` from its default "Update now" option. This fixed, + * short delay is a best-effort mitigation for the narrower "racing the + * literal process spawn" case D7 originally worried about — it does NOT + * detect or wait out a first-run onboarding screen (that would need + * ANSI-aware screen-state parsing, out of scope here). The onboarding-menu + * risk is real and unresolved; flagged for explicit human sign-off before + * merge, especially for the scratch-worktree path (see PR report). */ async function onCommitReviewFixWithAgent(payload: { tool: TerminalTabType; scratch: boolean }) { const prompt = buildReviewFixPrompt(commitReview.findings.value); @@ -1972,7 +2030,15 @@ async function onCommitReviewFixWithAgent(payload: { tool: TerminalTabType; scra let cwd: string | undefined; if (payload.scratch) { const scratch = await createAiTaskScratchWorktree(); - if (!scratch) return; + if (!scratch) { + // No repo/tab context to base the scratch on — surface it instead + // of silently doing nothing after the modal already closed. + // `reportAgentLaunchError` shows a generic "agent failed to open" + // message for agent tool types regardless of `err`'s content — the + // Error here is only for the console.error log. + reportAgentLaunchError(payload.tool, new Error("no active repo tab to base a scratch worktree on")); + return; + } cwd = scratch.path; } else { cwd = repoFolderPath.value ?? undefined; @@ -1981,7 +2047,13 @@ async function onCommitReviewFixWithAgent(payload: { tool: TerminalTabType; scra // `sessionId` is -1 until `terminalOpen` resolves inside `openTab` — // `openTerminalTab` only returns after that await settles, but guard // explicitly anyway (per plan) rather than relying on that implicitly. - if (!tab || tab.sessionId < 0) return; + if (!tab || tab.sessionId < 0) { + reportAgentLaunchError(payload.tool, new Error("terminal session unavailable")); + return; + } + // Best-effort readiness wait (verifier item #5) — gives the spawned + // process a moment past the raw PTY spawn before the prompt lands. + await new Promise((resolve) => setTimeout(resolve, 1000)); await termSessions.write(tab.sessionId, prompt); commitReview.armReReview(); } catch (err) { From 026c7a0265e5acb452f113d6ceeda854bb31e8ed Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Mon, 17 Aug 2026 12:24:41 +0200 Subject: [PATCH 18/46] refactor(commit-review): move the findings sort order into utils sortFindingsForReview lived in composables/useCommitReviewNav.ts, but utils/reviewFixPrompt.ts (a pure utils module) needed the same order and was importing it from there, inverting the established utils-do-not- depend-on-composables direction (Task 0's unifiedDiff.ts/editableTarget.ts precedent). Moves it to utils/reviewFindingsSort.ts; useCommitReviewNav.ts re-exports it verbatim for back-compat with CommitReviewModal.vue's import. disabled --- .../src/composables/useCommitReviewNav.ts | 22 ++++++--------- apps/desktop/src/utils/reviewFindingsSort.ts | 27 +++++++++++++++++++ apps/desktop/src/utils/reviewFixPrompt.ts | 2 +- 3 files changed, 36 insertions(+), 15 deletions(-) create mode 100644 apps/desktop/src/utils/reviewFindingsSort.ts diff --git a/apps/desktop/src/composables/useCommitReviewNav.ts b/apps/desktop/src/composables/useCommitReviewNav.ts index 9780c4bb..80dbacc3 100644 --- a/apps/desktop/src/composables/useCommitReviewNav.ts +++ b/apps/desktop/src/composables/useCommitReviewNav.ts @@ -11,20 +11,14 @@ import { ref, computed, nextTick, type ComputedRef, type Ref } from "vue"; import type { ReviewFinding } from "./usePrPreReview"; import type { CommitReviewAction } from "./commitReviewKeymap"; - -/** Lower rank = more severe, matching `CommitReviewModal.vue`'s sort. */ -const SEVERITY_RANK: Record = { risk: 0, suggestion: 1, nit: 2 }; - -/** Severity-sorted (risk > suggestion > nit), then confidence descending — - * the single source of truth for "finding order" shared by the modal's - * list and this composable's `N`/`P` cycling, so what you see in the - * modal is exactly what `N`/`P` steps through. */ -export function sortFindingsForReview(findings: ReviewFinding[]): ReviewFinding[] { - return [...findings].sort((a, b) => { - const rankDiff = SEVERITY_RANK[a.severity] - SEVERITY_RANK[b.severity]; - return rankDiff !== 0 ? rankDiff : b.confidence - a.confidence; - }); -} +// Moved to `utils/reviewFindingsSort.ts` (verifier low-priority item — a +// `utils/` module, `reviewFixPrompt.ts`, needed this sort order too, and +// utils importing from composables inverts Task 0's established +// dependency direction). Imported (for local use below) and re-exported +// verbatim for back-compat — `CommitReviewModal.vue` still imports it from +// this path. +import { sortFindingsForReview } from "../utils/reviewFindingsSort"; +export { sortFindingsForReview }; /** Minimal duck-typed handle for the mounted `DiffViewer` instance — just * enough to scroll to a finding, without this composable depending on the diff --git a/apps/desktop/src/utils/reviewFindingsSort.ts b/apps/desktop/src/utils/reviewFindingsSort.ts new file mode 100644 index 00000000..90d0e07a --- /dev/null +++ b/apps/desktop/src/utils/reviewFindingsSort.ts @@ -0,0 +1,27 @@ +/** + * reviewFindingsSort.ts + * + * Pure sort order for Commit Review findings — severity-sorted (risk > + * suggestion > nit), then confidence descending. The single source of + * truth for "finding order" shared by `CommitReviewModal.vue`'s list, + * `useCommitReviewNav`'s `N`/`P` cycling cursor, and `reviewFixPrompt.ts`'s + * agent-prompt ordering, so all three always agree. + * + * Lives in `utils/` (not `composables/`) — mirrors Task 0's + * `unifiedDiff.ts`/`editableTarget.ts` precedent: a pure, Vue-free, + * backend-free module so `utils/reviewFixPrompt.ts` (also pure) doesn't + * have to import from `composables/` to get it (verifier low-priority item + * — utils importing from composables inverts the established dependency + * direction). `useCommitReviewNav.ts` re-exports it verbatim for back-compat. + */ +import type { ReviewFinding } from "../composables/usePrPreReview"; + +/** Lower rank = more severe. */ +const SEVERITY_RANK: Record = { risk: 0, suggestion: 1, nit: 2 }; + +export function sortFindingsForReview(findings: ReviewFinding[]): ReviewFinding[] { + return [...findings].sort((a, b) => { + const rankDiff = SEVERITY_RANK[a.severity] - SEVERITY_RANK[b.severity]; + return rankDiff !== 0 ? rankDiff : b.confidence - a.confidence; + }); +} diff --git a/apps/desktop/src/utils/reviewFixPrompt.ts b/apps/desktop/src/utils/reviewFixPrompt.ts index 4df4bd5d..b1810522 100644 --- a/apps/desktop/src/utils/reviewFixPrompt.ts +++ b/apps/desktop/src/utils/reviewFixPrompt.ts @@ -12,7 +12,7 @@ * line editor. */ import type { ReviewFinding } from "../composables/usePrPreReview"; -import { sortFindingsForReview } from "../composables/useCommitReviewNav"; +import { sortFindingsForReview } from "./reviewFindingsSort"; const DEFAULT_MAX_FINDINGS = 25; From 8260c49e4947f3e323e240083551b660d287e467 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Mon, 17 Aug 2026 12:40:49 +0200 Subject: [PATCH 19/46] fix(commit-review): drop the scratch-worktree option from Fix with agent Deliberate scope-narrowing for PR2, not a bug fix. Manual QA against real claude/codex CLIs (see the previous commit's report) found that a brand-new scratch worktree always hits a first-run "trust this directory?" onboarding screen that misinterprets the piped fix prompt as menu navigation, which drove a real brew upgrade --cask codex attempt in testing. The user was shown this finding and decided to disable the scratch-worktree option for this PR rather than ship it with that risk. CommitReviewModal.vue's "Fix with agent" footer no longer offers a scratch checkbox; its fix-with-agent emit payload drops the scratch flag. App.vue's onCommitReviewFixWithAgent always targets the current repo (already trusted, no onboarding screen) and no longer calls createAiTaskScratchWorktree, which stays untouched for its other caller, confirmNewAiTask ("New AI task"). Removed the now-unreachable commitReview.fixInScratch i18n key from all 5 locales after confirming it was not used anywhere else. Updated ROADMAP.md's Fix with agent bullet to record the cut and the follow-up: revisit scratch-worktree support once there is a real fix for the onboarding-trust-screen problem (pre-trusting the directory before launching the agent, or detecting the onboarding screen before writing). disabled --- ROADMAP.md | 2 +- apps/desktop/src/App.vue | 68 ++++++++----------- .../src/components/CommitReviewModal.vue | 33 ++++----- .../__tests__/CommitReviewModal.test.ts | 26 ++++--- apps/desktop/src/locales/en.ts | 1 - apps/desktop/src/locales/es.ts | 1 - apps/desktop/src/locales/fr.ts | 1 - apps/desktop/src/locales/pt-BR.ts | 1 - apps/desktop/src/locales/zh-CN.ts | 1 - 9 files changed, 61 insertions(+), 73 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index 90857e3c..68cbbb78 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -16,7 +16,7 @@ _Inspired by [git-lrc](https://github.com/HexmosTech/git-lrc) (HexmosTech). Comm - **Review staged changes** — one button in the commit area: AI pass over the staged diff, inline findings with severity badges anchored in the diff + a short summary. Generalize `usePrHunkCritique` from PR hunks to any `GitDiff` — the same engine as the v3.5.0 pre-review pass, pointed at the index - **Issue navigation** — cycle finding-to-finding (reuses the v3.5.0 keyboard model), per-file finding counts in the staged list -- **Fix with agent** — git-lrc makes you copy-paste issues back to your agent; we pipe them: "Fix with agent" sends the findings to Claude Code / opencode / Codex (Agent Sessions), optionally in an AI-task scratch worktree; re-review triggers on the next staging change +- **Fix with agent** — git-lrc makes you copy-paste issues back to your agent; we pipe them: "Fix with agent" sends the findings to Claude Code / opencode / Codex (Agent Sessions), always against the current repo; re-review triggers on the next staging change. The originally-planned "optionally in an AI-task scratch worktree" variant was cut from PR2 after manual QA against real claude/codex CLIs found a brand-new scratch worktree always hits a first-run "trust this directory?" onboarding screen that misinterprets the piped prompt as menu navigation (drove a real `brew upgrade --cask codex` in testing). Revisit once there's a real fix — pre-trusting the directory before launching the agent, or detecting the onboarding screen before writing — tracked as a v3.7.x/v3.8.0 follow-up - **Iterations & coverage** — track review→fix→review cycles and the share of the final staged diff already reviewed (`iter:N`, `coverage:X%`) - **Review / Vouch / Skip** — explicit three-state decision at commit time, non-blocking (same UX contract as the v3.5.0 secrets scanner): reviewed by AI, vouched personally, or skipped — recorded as a commit trailer `GitWand-Review: ran|vouched|skipped (iter:N, coverage:X%)` via the existing trailers support (v1.9.0), so the team sees review status right in `git log` - **Opt-in & scoped** — per-repo enable in `.gitwandrc` + Settings; optional pre-commit hook wiring via Settings > Hooks alongside the v3.5.0 scanner diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index 301db21a..61106934 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -1995,54 +1995,40 @@ async function confirmNewAiTask(name: string) { * findings prompt into a fresh agent PTY WITHOUT pressing Enter (plan * decision D7 — no `terminal_open` "initial prompt" param exists, and typing * without submitting avoids racing the agent TUI's boot while keeping "let - * an agent edit my files" a deliberate human gesture). Optionally opens the - * agent in a scratch worktree first (reuses `createAiTaskScratchWorktree`). + * an agent edit my files" a deliberate human gesture). * - * Verifier item #5 — manual QA performed against real `claude` and `codex` - * CLIs (via the dev-server's real `node-pty` backend, the same one - * `pnpm dev:web` uses) confirmed: once an agent has reached its normal - * ready-to-chat input state, writing this whole multi-line, trailing-\n - * prompt as one burst lands as UNSENT multi-line input text (verified for - * `claude` — its bracketed-paste-mode input box shows every line, with no - * submission and no response activity for several seconds after). That - * part of decision D7's assumption holds. + * Always targets the CURRENT repo — deliberately scope-narrowed for PR2 + * (not a bug fix): this used to optionally open the agent in a fresh + * scratch worktree via `createAiTaskScratchWorktree`, but manual QA against + * real `claude`/`codex` CLIs found that a brand-new working directory + * (exactly what a scratch worktree always is) hits a first-run "trust this + * directory?" onboarding screen before the agent's normal input box exists. + * Writing newline-bearing input into THAT screen doesn't "type unsent + * text" — it drives Enter-confirms-the-highlighted-option menu navigation, + * and in testing this went as far as `codex` starting a real `brew upgrade + * --cask codex` from its default "Update now" option. The current repo is + * already trusted (no onboarding screen), so this path is safe; the + * scratch-worktree option is removed until there's a real fix — e.g. + * pre-trusting the directory before launching the agent, or detecting the + * onboarding screen before writing — tracked as a roadmap follow-up. + * `createAiTaskScratchWorktree` itself is untouched and still used by the + * existing "New AI task" button (`confirmNewAiTask`). * - * BUT: a brand-new working directory (exactly what "in a scratch worktree" - * always is) makes both `claude` and `codex` show a first-run "trust this - * directory?" onboarding prompt before their normal input box exists, and - * for `codex` a subsequent "update available" prompt can follow. Writing - * newline-bearing input into THOSE screens does not "type unsent text" — - * it drives their Enter-confirms-the-highlighted-option menu navigation. - * In manual testing this went as far as `codex` starting a real `brew - * upgrade --cask codex` from its default "Update now" option. This fixed, - * short delay is a best-effort mitigation for the narrower "racing the - * literal process spawn" case D7 originally worried about — it does NOT - * detect or wait out a first-run onboarding screen (that would need - * ANSI-aware screen-state parsing, out of scope here). The onboarding-menu - * risk is real and unresolved; flagged for explicit human sign-off before - * merge, especially for the scratch-worktree path (see PR report). + * Verifier item #5 — manual QA performed via the dev-server's real + * `node-pty` backend (the same one `pnpm dev:web` uses) confirmed: once an + * agent has reached its normal ready-to-chat input state, writing this + * whole multi-line, trailing-\n prompt as one burst lands as UNSENT + * multi-line input text (verified for `claude` — its bracketed-paste-mode + * input box shows every line, with no submission and no response activity + * for several seconds after). That part of decision D7's assumption holds + * for the current-repo path this function now exclusively uses. */ -async function onCommitReviewFixWithAgent(payload: { tool: TerminalTabType; scratch: boolean }) { +async function onCommitReviewFixWithAgent(payload: { tool: TerminalTabType }) { const prompt = buildReviewFixPrompt(commitReview.findings.value); if (!prompt) return; showCommitReviewModal.value = false; try { - let cwd: string | undefined; - if (payload.scratch) { - const scratch = await createAiTaskScratchWorktree(); - if (!scratch) { - // No repo/tab context to base the scratch on — surface it instead - // of silently doing nothing after the modal already closed. - // `reportAgentLaunchError` shows a generic "agent failed to open" - // message for agent tool types regardless of `err`'s content — the - // Error here is only for the console.error log. - reportAgentLaunchError(payload.tool, new Error("no active repo tab to base a scratch worktree on")); - return; - } - cwd = scratch.path; - } else { - cwd = repoFolderPath.value ?? undefined; - } + const cwd = repoFolderPath.value ?? undefined; const tab = await openTerminalTab(cwd, payload.tool); // `sessionId` is -1 until `terminalOpen` resolves inside `openTab` — // `openTerminalTab` only returns after that await settles, but guard diff --git a/apps/desktop/src/components/CommitReviewModal.vue b/apps/desktop/src/components/CommitReviewModal.vue index 9ddfb3ef..982cedbb 100644 --- a/apps/desktop/src/components/CommitReviewModal.vue +++ b/apps/desktop/src/components/CommitReviewModal.vue @@ -4,8 +4,19 @@ * * Task 1b (v3.7.0) — summary + severity-sorted finding list for the * staged-diff Commit Review pass. Modelled on `SecretsFindingsModal.vue`. - * Task 3 adds "Fix with agent" (tool + scratch-worktree picker); Task 4 adds - * the iteration/coverage line. + * Task 3 adds "Fix with agent" (tool picker); Task 4 adds the + * iteration/coverage line. + * + * Scope-narrowed for PR2 (deliberate, not a bug fix): "Fix with agent" no + * longer offers a scratch-worktree option. Manual QA against real + * claude/codex CLIs found that a brand-new scratch worktree always hits a + * first-run "trust this directory?" onboarding screen that misinterprets + * the piped prompt as menu navigation (it drove a real `brew upgrade + * --cask codex` in testing, from the CLI's default "Update now" option). + * "Fix with agent" only ever targets the CURRENT repo now (already + * trusted, no onboarding screen). Revisit scratch-worktree support once + * there's a real fix — e.g. pre-trusting the directory before launching + * the agent, or detecting the onboarding screen before writing. */ import { computed, ref } from "vue"; import BaseModal from "./BaseModal.vue"; @@ -33,7 +44,7 @@ const emit = defineEmits<{ jump: [id: string]; dismiss: [id: string]; close: []; - "fix-with-agent": [{ tool: TerminalTabType; scratch: boolean }]; + "fix-with-agent": [{ tool: TerminalTabType }]; }>(); const { t } = useI18n(); @@ -45,7 +56,6 @@ const FIX_AGENT_TOOLS: Extract "opencode", ]; const selectedTool = ref("claude"); -const fixInScratch = ref(false); const TOOL_LABEL_KEY: Record<(typeof FIX_AGENT_TOOLS)[number], "commitReview.toolClaude" | "commitReview.toolCodex" | "commitReview.toolOpencode"> = { claude: "commitReview.toolClaude", @@ -59,7 +69,7 @@ function toolLabel(tool: (typeof FIX_AGENT_TOOLS)[number]): string { function onFixWithAgentClick() { if (!props.findings.length) return; - emit("fix-with-agent", { tool: selectedTool.value, scratch: fixInScratch.value }); + emit("fix-with-agent", { tool: selectedTool.value }); } const SEVERITY_LABEL_KEY: Record = { @@ -126,10 +136,6 @@ const sortedFindings = computed(() => sortFindingsForReview(props.findings)); {{ toolLabel(tool) }} - + + + +
{{ t("common.loading") }}
@@ -383,8 +467,8 @@ onMounted(() => { margin-bottom: 12px; } -/* Secrets pre-commit hook — dedicated section (v3.5.0) */ -.hp-secrets { +/* GitWand-managed pre-commit hook rows — secrets + commit review (v3.7.0, Task 6) */ +.hp-hookrow { display: flex; align-items: center; justify-content: space-between; @@ -396,32 +480,32 @@ onMounted(() => { margin-bottom: 12px; } -.hp-secrets-info { +.hp-hookrow-info { display: flex; flex-direction: column; gap: 2px; min-width: 0; } -.hp-secrets-title { +.hp-hookrow-title { font-size: 12px; font-weight: 600; color: var(--color-text-primary); } -.hp-secrets-desc { +.hp-hookrow-desc { font-size: 11px; color: var(--color-text-muted); } -.hp-secrets-actions { +.hp-hookrow-actions { display: flex; align-items: center; gap: 8px; flex-shrink: 0; } -.hp-secrets-remove { +.hp-hookrow-remove { color: var(--color-danger, #e53e3e); } diff --git a/apps/desktop/src/components/SettingsPanel.vue b/apps/desktop/src/components/SettingsPanel.vue index 6bb854e1..21619ad9 100644 --- a/apps/desktop/src/components/SettingsPanel.vue +++ b/apps/desktop/src/components/SettingsPanel.vue @@ -2768,6 +2768,7 @@ function deleteReleaseNoteTemplate(id: string) { {{ t('settings.commitReview.enabled') }} {{ t('settings.commitReview.enabledHint') }} + {{ t('settings.commitReview.rcOverrideHint') }}
{{ error }}
+ +
+
+ + {{ t("hooks.foreignTitle") }} + {{ t("hooks.foreignBadge") }} + + {{ t("hooks.foreignDescription") }} +
+
+ @@ -656,6 +684,18 @@ onMounted(() => { color: var(--color-text-muted); } +/* v3.7.0 review-round fix (finding #7) — foreign pre-commit hook warning */ +.hp-badge--warn { + background: rgba(217, 119, 6, 0.15); + color: var(--color-warning, #d97706); + margin-left: 6px; +} + +.hp-hookrow--foreign { + background: rgba(217, 119, 6, 0.08); + border-color: var(--color-warning, #d97706); +} + /* Form */ .hp-form { display: flex; diff --git a/apps/desktop/src/locales/en.ts b/apps/desktop/src/locales/en.ts index d393cee6..136438bd 100644 --- a/apps/desktop/src/locales/en.ts +++ b/apps/desktop/src/locales/en.ts @@ -1578,7 +1578,8 @@ const en = { secretsInstall: "Install", secretsRemove: "Remove", secretsInstallConfirmTitle: "Install the secrets pre-commit hook?", - secretsInstallConfirmMessage: "This writes .git/hooks/pre-commit and shells out to \u201cnpx @gitwand/cli scan\u201d on every commit made from the terminal. It can always be bypassed with \u201cgit commit --no-verify\u201d. If a pre-commit hook already exists and is managed by GitWand, this adds the secrets section to it without touching the commit review section. If it is not managed by GitWand, it will be overwritten.", + secretsInstallConfirmMessage: "This writes .git/hooks/pre-commit and shells out to \u201cnpx @gitwand/cli scan\u201d on every commit made from the terminal. It can always be bypassed with \u201cgit commit --no-verify\u201d. If a pre-commit hook already exists and is managed by GitWand, this adds the secrets section to it without touching the commit review section.", + secretsInstallConfirmMessageForeign: "This OVERWRITES the pre-commit hook already installed at .git/hooks/pre-commit, which is not managed by GitWand. The new hook writes .git/hooks/pre-commit and shells out to \u201cnpx @gitwand/cli scan\u201d on every commit made from the terminal. It can always be bypassed with \u201cgit commit --no-verify\u201d.", secretsRemoveConfirmTitle: "Remove the secrets pre-commit hook?", secretsRemoveConfirmMessage: "This removes the secrets section from .git/hooks/pre-commit. Terminal commits will no longer be scanned for secrets. If the commit review section is not also installed, the file is deleted entirely.", errorSecretsInstall: "Failed to install the secrets hook: {0}", @@ -1592,9 +1593,14 @@ const en = { reviewInstall: "Install", reviewRemove: "Remove", reviewInstallConfirmTitle: "Install the commit review reminder hook?", - reviewInstallConfirmMessage: "This writes .git/hooks/pre-commit (or adds to the existing GitWand-managed one) to print a reminder on every terminal commit that Commit Review didn't run for it. It never blocks the commit. If a pre-commit hook already exists and isn't GitWand-managed, it will be overwritten.", + reviewInstallConfirmMessage: "This writes .git/hooks/pre-commit (or adds to the existing GitWand-managed one) to print a reminder on every terminal commit that Commit Review didn't run for it. It never blocks the commit.", + reviewInstallConfirmMessageForeign: "This OVERWRITES the pre-commit hook already installed at .git/hooks/pre-commit, which is not managed by GitWand. The new hook prints a reminder on every terminal commit that Commit Review didn't run for it. It never blocks the commit.", reviewRemoveConfirmTitle: "Remove the commit review reminder hook?", reviewRemoveConfirmMessage: "This removes the commit review section from .git/hooks/pre-commit, deleting the file entirely if the secrets hook isn't also installed.", + // v3.7.0 review-round fix (finding #7) — a foreign (non-GitWand) pre-commit hook + foreignTitle: "A pre-commit hook not managed by GitWand is installed", + foreignDescription: "Installing Secrets or Commit Review below will overwrite this existing script.", + foreignBadge: "Foreign hook", }, // \u2500\u2500\u2500 Workspaces \u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500\u2500 diff --git a/apps/desktop/src/locales/es.ts b/apps/desktop/src/locales/es.ts index cb440ca2..bfff0160 100644 --- a/apps/desktop/src/locales/es.ts +++ b/apps/desktop/src/locales/es.ts @@ -1546,7 +1546,8 @@ const es: Locale = { secretsInstall: "Instalar", secretsRemove: "Quitar", secretsInstallConfirmTitle: "¿Instalar el hook pre-commit de secretos?", - secretsInstallConfirmMessage: "Esto escribe .git/hooks/pre-commit y ejecuta «npx @gitwand/cli scan» en cada commit hecho desde la terminal. Siempre se puede omitir con «git commit --no-verify». Si ya existe un hook pre-commit gestionado por GitWand, esto añade la sección de secretos sin tocar la sección de revisión de commit. Si no está gestionado por GitWand, será sobrescrito.", + secretsInstallConfirmMessage: "Esto escribe .git/hooks/pre-commit y ejecuta «npx @gitwand/cli scan» en cada commit hecho desde la terminal. Siempre se puede omitir con «git commit --no-verify». Si ya existe un hook pre-commit gestionado por GitWand, esto añade la sección de secretos sin tocar la sección de revisión de commit.", + secretsInstallConfirmMessageForeign: "Esto SOBRESCRIBE el hook pre-commit ya instalado en .git/hooks/pre-commit, que no está gestionado por GitWand. El nuevo hook escribe .git/hooks/pre-commit y ejecuta «npx @gitwand/cli scan» en cada commit hecho desde la terminal. Siempre se puede omitir con «git commit --no-verify».", secretsRemoveConfirmTitle: "¿Quitar el hook pre-commit de secretos?", secretsRemoveConfirmMessage: "Esto quita la sección de secretos de .git/hooks/pre-commit. Los commits hechos desde la terminal ya no se escanearán en busca de secretos. Si la sección de revisión de commit tampoco está instalada, el archivo se elimina por completo.", errorSecretsInstall: "Error al instalar el hook de secretos: {0}", @@ -1560,9 +1561,14 @@ const es: Locale = { reviewInstall: "Instalar", reviewRemove: "Quitar", reviewInstallConfirmTitle: "¿Instalar el hook de recordatorio de revisión de commit?", - reviewInstallConfirmMessage: "Esto escribe .git/hooks/pre-commit (o lo añade al script de GitWand existente) para imprimir un recordatorio en cada commit desde la terminal indicando que la Revisión de commit no se ejecutó. Nunca bloquea el commit. Si ya existe un hook pre-commit que no está gestionado por GitWand, se sobrescribirá.", + reviewInstallConfirmMessage: "Esto escribe .git/hooks/pre-commit (o lo añade al script de GitWand existente) para imprimir un recordatorio en cada commit desde la terminal indicando que la Revisión de commit no se ejecutó. Nunca bloquea el commit.", + reviewInstallConfirmMessageForeign: "Esto SOBRESCRIBE el hook pre-commit ya instalado en .git/hooks/pre-commit, que no está gestionado por GitWand. El nuevo hook imprime un recordatorio en cada commit desde la terminal indicando que la Revisión de commit no se ejecutó. Nunca bloquea el commit.", reviewRemoveConfirmTitle: "¿Quitar el hook de recordatorio de revisión de commit?", reviewRemoveConfirmMessage: "Esto quita la sección de revisión de commit de .git/hooks/pre-commit, eliminando el archivo por completo si el hook de secretos tampoco está instalado.", + // v3.7.0 review-round fix (finding #7) — hook pre-commit externo (no gestionado por GitWand) + foreignTitle: "Hay un hook pre-commit no gestionado por GitWand instalado", + foreignDescription: "Instalar Secretos o Revisión de commit abajo sobrescribirá este script existente.", + foreignBadge: "Hook externo", }, workspace: { diff --git a/apps/desktop/src/locales/fr.ts b/apps/desktop/src/locales/fr.ts index f5154644..42b25e89 100644 --- a/apps/desktop/src/locales/fr.ts +++ b/apps/desktop/src/locales/fr.ts @@ -1555,7 +1555,8 @@ const fr: Locale = { secretsInstall: "Installer", secretsRemove: "Retirer", secretsInstallConfirmTitle: "Installer le hook pre-commit de secrets ?", - secretsInstallConfirmMessage: "Ceci écrit .git/hooks/pre-commit et exécute « npx @gitwand/cli scan » à chaque commit fait depuis le terminal. Toujours contournable avec « git commit --no-verify ». Si un hook pre-commit existe déjà et est géré par GitWand, ceci ajoute la section secrets sans toucher à la section de revue de commit. S'il n'est pas géré par GitWand, il sera écrasé.", + secretsInstallConfirmMessage: "Ceci écrit .git/hooks/pre-commit et exécute « npx @gitwand/cli scan » à chaque commit fait depuis le terminal. Toujours contournable avec « git commit --no-verify ». Si un hook pre-commit existe déjà et est géré par GitWand, ceci ajoute la section secrets sans toucher à la section de revue de commit.", + secretsInstallConfirmMessageForeign: "Ceci ÉCRASE le hook pre-commit déjà installé à .git/hooks/pre-commit, qui n'est pas géré par GitWand. Le nouveau hook écrit .git/hooks/pre-commit et exécute « npx @gitwand/cli scan » à chaque commit fait depuis le terminal. Toujours contournable avec « git commit --no-verify ».", secretsRemoveConfirmTitle: "Retirer le hook pre-commit de secrets ?", secretsRemoveConfirmMessage: "Ceci retire la section secrets de .git/hooks/pre-commit. Les commits faits depuis le terminal ne seront plus analysés pour des secrets. Si la section de revue de commit n'est pas installée non plus, le fichier est supprimé entièrement.", errorSecretsInstall: "Échec de l'installation du hook de secrets : {0}", @@ -1569,9 +1570,14 @@ const fr: Locale = { reviewInstall: "Installer", reviewRemove: "Supprimer", reviewInstallConfirmTitle: "Installer le hook de rappel de revue de commit ?", - reviewInstallConfirmMessage: "Ceci écrit .git/hooks/pre-commit (ou l'ajoute au script GitWand existant) pour afficher un rappel à chaque commit terminal indiquant que la Revue de commit ne s'est pas exécutée. Le commit n'est jamais bloqué. Si un hook pre-commit existe déjà et n'est pas géré par GitWand, il sera écrasé.", + reviewInstallConfirmMessage: "Ceci écrit .git/hooks/pre-commit (ou l'ajoute au script GitWand existant) pour afficher un rappel à chaque commit terminal indiquant que la Revue de commit ne s'est pas exécutée. Le commit n'est jamais bloqué.", + reviewInstallConfirmMessageForeign: "Ceci ÉCRASE le hook pre-commit déjà installé à .git/hooks/pre-commit, qui n'est pas géré par GitWand. Le nouveau hook affiche un rappel à chaque commit terminal indiquant que la Revue de commit ne s'est pas exécutée. Le commit n'est jamais bloqué.", reviewRemoveConfirmTitle: "Supprimer le hook de rappel de revue de commit ?", reviewRemoveConfirmMessage: "Ceci retire la section de revue de commit de .git/hooks/pre-commit, en supprimant le fichier entièrement si le hook de secrets n'est pas non plus installé.", + // v3.7.0 review-round fix (finding #7) — hook pre-commit externe (non géré par GitWand) + foreignTitle: "Un hook pre-commit non géré par GitWand est installé", + foreignDescription: "Installer Secrets ou Revue de commit ci-dessous écrasera ce script existant.", + foreignBadge: "Hook externe", }, workspace: { diff --git a/apps/desktop/src/locales/pt-BR.ts b/apps/desktop/src/locales/pt-BR.ts index 63ce4c19..aeb3b276 100644 --- a/apps/desktop/src/locales/pt-BR.ts +++ b/apps/desktop/src/locales/pt-BR.ts @@ -1546,7 +1546,8 @@ const ptBR: Locale = { secretsInstall: "Instalar", secretsRemove: "Remover", secretsInstallConfirmTitle: "Instalar o hook pre-commit de segredos?", - secretsInstallConfirmMessage: "Isso grava .git/hooks/pre-commit e executa “npx @gitwand/cli scan” em cada commit feito pelo terminal. Sempre pode ser ignorado com “git commit --no-verify”. Se já existir um hook pre-commit gerenciado pelo GitWand, isso adiciona a seção de segredos sem alterar a seção de revisão de commit. Se não for gerenciado pelo GitWand, ele será sobrescrito.", + secretsInstallConfirmMessage: "Isso grava .git/hooks/pre-commit e executa “npx @gitwand/cli scan” em cada commit feito pelo terminal. Sempre pode ser ignorado com “git commit --no-verify”. Se já existir um hook pre-commit gerenciado pelo GitWand, isso adiciona a seção de segredos sem alterar a seção de revisão de commit.", + secretsInstallConfirmMessageForeign: "Isso SOBRESCREVE o hook pre-commit já instalado em .git/hooks/pre-commit, que não é gerenciado pelo GitWand. O novo hook grava .git/hooks/pre-commit e executa “npx @gitwand/cli scan” em cada commit feito pelo terminal. Sempre pode ser ignorado com “git commit --no-verify”.", secretsRemoveConfirmTitle: "Remover o hook pre-commit de segredos?", secretsRemoveConfirmMessage: "Isso remove a seção de segredos de .git/hooks/pre-commit. Commits feitos pelo terminal deixarão de ser verificados em busca de segredos. Se a seção de revisão de commit também não estiver instalada, o arquivo é apagado por completo.", errorSecretsInstall: "Falha ao instalar o hook de segredos: {0}", @@ -1560,9 +1561,14 @@ const ptBR: Locale = { reviewInstall: "Instalar", reviewRemove: "Remover", reviewInstallConfirmTitle: "Instalar o hook de lembrete de revisão de commit?", - reviewInstallConfirmMessage: "Isso grava .git/hooks/pre-commit (ou adiciona ao script GitWand existente) para imprimir um lembrete em cada commit feito pelo terminal informando que a Revisão de commit não foi executada. Nunca bloqueia o commit. Se já existir um hook pre-commit que não é gerenciado pelo GitWand, ele será sobrescrito.", + reviewInstallConfirmMessage: "Isso grava .git/hooks/pre-commit (ou adiciona ao script GitWand existente) para imprimir um lembrete em cada commit feito pelo terminal informando que a Revisão de commit não foi executada. Nunca bloqueia o commit.", + reviewInstallConfirmMessageForeign: "Isso SOBRESCREVE o hook pre-commit já instalado em .git/hooks/pre-commit, que não é gerenciado pelo GitWand. O novo hook imprime um lembrete em cada commit feito pelo terminal informando que a Revisão de commit não foi executada. Nunca bloqueia o commit.", reviewRemoveConfirmTitle: "Remover o hook de lembrete de revisão de commit?", reviewRemoveConfirmMessage: "Isso remove a seção de revisão de commit de .git/hooks/pre-commit, apagando o arquivo por completo se o hook de segredos também não estiver instalado.", + // v3.7.0 review-round fix (finding #7) — hook pre-commit externo (não gerenciado pelo GitWand) + foreignTitle: "Um hook pre-commit não gerenciado pelo GitWand está instalado", + foreignDescription: "Instalar Segredos ou Revisão de commit abaixo sobrescreverá esse script existente.", + foreignBadge: "Hook externo", }, workspace: { diff --git a/apps/desktop/src/locales/zh-CN.ts b/apps/desktop/src/locales/zh-CN.ts index 221d1682..4871cffb 100644 --- a/apps/desktop/src/locales/zh-CN.ts +++ b/apps/desktop/src/locales/zh-CN.ts @@ -1533,7 +1533,8 @@ const zhCN: Locale = { secretsInstall: "安装", secretsRemove: "移除", secretsInstallConfirmTitle: "安装密钥 pre-commit 钩子？", - secretsInstallConfirmMessage: "这将写入 .git/hooks/pre-commit，并在每次从终端提交时运行“npx @gitwand/cli scan”。始终可以用“git commit --no-verify”绕过。如果已存在一个由 GitWand 管理的 pre-commit 钩子，这会添加密钥部分而不影响提交审查部分。如果它不是由 GitWand 管理的，它将被覆盖。", + secretsInstallConfirmMessage: "这将写入 .git/hooks/pre-commit，并在每次从终端提交时运行“npx @gitwand/cli scan”。始终可以用“git commit --no-verify”绕过。如果已存在一个由 GitWand 管理的 pre-commit 钩子，这会添加密钥部分而不影响提交审查部分。", + secretsInstallConfirmMessageForeign: "这将覆盖已安装在 .git/hooks/pre-commit 的钩子，该钩子并非由 GitWand 管理。新钩子会写入 .git/hooks/pre-commit，并在每次从终端提交时运行“npx @gitwand/cli scan”。始终可以用“git commit --no-verify”绕过。", secretsRemoveConfirmTitle: "移除密钥 pre-commit 钩子？", secretsRemoveConfirmMessage: "这将从 .git/hooks/pre-commit 中移除密钥部分。从终端提交的更改将不再进行密钥扫描。如果提交审查部分也未安装，该文件将被彻底删除。", errorSecretsInstall: "安装密钥钩子失败：{0}", @@ -1547,9 +1548,14 @@ const zhCN: Locale = { reviewInstall: "安装", reviewRemove: "移除", reviewInstallConfirmTitle: "安装提交审查提醒钩子？", - reviewInstallConfirmMessage: "这会写入 .git/hooks/pre-commit（或添加到现有的 GitWand 脚本中），在每次终端提交时打印一条提醒，说明提交审查未运行。此操作绝不会阻止提交。如果已存在一个非 GitWand 管理的 pre-commit 钩子，它将被覆盖。", + reviewInstallConfirmMessage: "这会写入 .git/hooks/pre-commit（或添加到现有的 GitWand 脚本中），在每次终端提交时打印一条提醒，说明提交审查未运行。此操作绝不会阻止提交。", + reviewInstallConfirmMessageForeign: "这将覆盖已安装在 .git/hooks/pre-commit 的钩子，该钩子并非由 GitWand 管理。新钩子会在每次终端提交时打印一条提醒，说明提交审查未运行。此操作绝不会阻止提交。", reviewRemoveConfirmTitle: "移除提交审查提醒钩子？", reviewRemoveConfirmMessage: "这会从 .git/hooks/pre-commit 中移除提交审查部分；如果密钥钩子也未安装，则会彻底删除该文件。", + // v3.7.0 review-round fix (finding #7) — 非 GitWand 管理的 pre-commit 钩子 + foreignTitle: "检测到一个非 GitWand 管理的 pre-commit 钩子", + foreignDescription: "安装下方的密钥或提交审查钩子将覆盖此现有脚本。", + foreignBadge: "外部钩子", }, workspace: { diff --git a/apps/desktop/src/utils/__tests__/gitwandHook.test.ts b/apps/desktop/src/utils/__tests__/gitwandHook.test.ts index 73c9302c..9ba0cd37 100644 --- a/apps/desktop/src/utils/__tests__/gitwandHook.test.ts +++ b/apps/desktop/src/utils/__tests__/gitwandHook.test.ts @@ -3,6 +3,7 @@ import { GITWAND_HOOK_MARKER, buildGitwandHookScript, parseGitwandHookSections, + classifyPreCommitHook, type HookSections, } from "../gitwandHook"; import { buildSecretsHookScript } from "../secretsHook"; @@ -93,3 +94,64 @@ describe("neither section — an empty composable script still carries the v2 ma expect(parseGitwandHookSections(script)).toEqual({ secrets: false, review: false }); }); }); + +// v3.7.0 review-round fix (finding #7) — a foreign (non-GitWand) pre-commit +// hook collapsed to the exact same "none" state as no hook at all, so the UI +// showed no warning that Install would OVERWRITE the user's own script. +describe("classifyPreCommitHook", () => { + it("null (unreadable or absent) classifies as none, with both sections false", () => { + expect(classifyPreCommitHook(null)).toEqual({ + kind: "none", + sections: { secrets: false, review: false }, + }); + }); + + it("an empty string classifies as none", () => { + expect(classifyPreCommitHook("")).toEqual({ + kind: "none", + sections: { secrets: false, review: false }, + }); + }); + + it("a whitespace-only string classifies as none", () => { + expect(classifyPreCommitHook(" \n\t \n")).toEqual({ + kind: "none", + sections: { secrets: false, review: false }, + }); + }); + + const combos: HookSections[] = [ + { secrets: false, review: false }, + { secrets: true, review: false }, + { secrets: false, review: true }, + { secrets: true, review: true }, + ]; + for (const sections of combos) { + it(`a v2 GitWand script (secrets=${sections.secrets} review=${sections.review}) classifies as gitwand with the right sections`, () => { + const script = buildGitwandHookScript(sections); + expect(classifyPreCommitHook(script)).toEqual({ kind: "gitwand", sections }); + }); + } + + it("a v1 secrets-only script classifies as gitwand with {secrets:true, review:false} (migration regression guard)", () => { + const v1Script = buildSecretsHookScript(); + expect(classifyPreCommitHook(v1Script)).toEqual({ + kind: "gitwand", + sections: { secrets: true, review: false }, + }); + }); + + it("a hand-written, unrelated script classifies as foreign", () => { + expect(classifyPreCommitHook("#!/bin/sh\nnpm test\n")).toEqual({ + kind: "foreign", + sections: { secrets: false, review: false }, + }); + }); + + it("a script that merely mentions 'gitwand' in a comment (no marker) still classifies as foreign — no fuzzy matching", () => { + expect(classifyPreCommitHook("#!/bin/sh\n# note: unrelated to gitwand\nnpm test\n")).toEqual({ + kind: "foreign", + sections: { secrets: false, review: false }, + }); + }); +}); diff --git a/apps/desktop/src/utils/gitwandHook.ts b/apps/desktop/src/utils/gitwandHook.ts index 26635557..11983ad6 100644 --- a/apps/desktop/src/utils/gitwandHook.ts +++ b/apps/desktop/src/utils/gitwandHook.ts @@ -107,3 +107,39 @@ export function parseGitwandHookSections(content: string): HookSections | null { // Re-exported so call sites that only need to detect a legacy v1 script don't have to import // from both modules. export { SECRETS_HOOK_MARKER }; + +/** + * v3.7.0 review-round fix (finding #7) — classifies WHAT is installed at + * `.git/hooks/pre-commit`, distinguishing a foreign (non-GitWand) script from + * no hook at all. Before this, `parseGitwandHookSections` returning `null` + * for both cases meant a hand-written pre-commit hook looked identical to an + * empty hooks directory in the UI, and `writeGitwandHook` would silently + * OVERWRITE that foreign script on Install with no distinct warning. + */ +export type PreCommitHookKind = "none" | "gitwand" | "foreign"; + +export interface PreCommitHookState { + kind: PreCommitHookKind; + sections: HookSections; +} + +/** + * `content === null` means "unreadable or absent". A non-empty script that + * `parseGitwandHookSections` does not recognize is "foreign": GitWand would + * OVERWRITE it on install, so the UI must say so specifically rather than + * showing the same "nothing installed" state as an empty hooks dir. + */ +export function classifyPreCommitHook(content: string | null): PreCommitHookState { + const NO_SECTIONS: HookSections = { secrets: false, review: false }; + + if (content === null || content.trim() === "") { + return { kind: "none", sections: NO_SECTIONS }; + } + + const parsed = parseGitwandHookSections(content); + if (parsed !== null) { + return { kind: "gitwand", sections: parsed }; + } + + return { kind: "foreign", sections: NO_SECTIONS }; +} From 7867dccb7ee08957274d565306cf1563a0ce61d5 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 18:41:00 +0200 Subject: [PATCH 38/46] docs(commit-review): describe the real byte-budget guarantee COMMIT_REVIEW_MAX_BYTES's doc comment claimed a "hard cap", but the enforcement admits a file slice BEFORE decrementing the budget, so a single file whose diff already exceeds the whole budget is still admitted whole, only files after it are excluded. This is tested and intentional (see the test named "truncates by the byte budget when a single file's diff exceeds it, excluding files after it") -- only the comment was wrong. Comment-only change: pnpm test is green with zero test edits, which is itself the proof nothing behavioral moved. --- apps/desktop/src/composables/useCommitReview.ts | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/composables/useCommitReview.ts b/apps/desktop/src/composables/useCommitReview.ts index fc47819b..f0c2df85 100644 --- a/apps/desktop/src/composables/useCommitReview.ts +++ b/apps/desktop/src/composables/useCommitReview.ts @@ -51,9 +51,19 @@ import { export const COMMIT_REVIEW_MAX_FILES = 40; /** - * Hard cap on the total staged-diff bytes sent through the review pass, - * applied in file-slice order (earlier files in the diff win). Same D12 - * caveat as `COMMIT_REVIEW_MAX_FILES`. + * v3.7.0 review-round fix (finding #9) — this is NOT a hard cap on the total + * bytes reviewed, unlike `COMMIT_REVIEW_MAX_FILES` above (which genuinely is + * one: `slices.slice(0, config.maxFiles)`). The budget is only checked + * BEFORE admitting each file slice, in file-slice order (earlier files in + * the diff win): a single file whose own diff already exceeds the whole + * budget is still admitted whole, and only files AFTER it are excluded. This + * is deliberate, not a bug — see the test named "truncates by the byte + * budget when a single file's diff exceeds it, excluding files after it" + * (`useCommitReview.test.ts`) — so a huge single-file staged change still + * gets reviewed rather than silently skipped entirely. `truncated` is set + * either way, so the UI always knows part of the staged diff went + * unreviewed. Same D12 caveat (this number is a guess) as + * `COMMIT_REVIEW_MAX_FILES`. */ export const COMMIT_REVIEW_MAX_BYTES = 400_000; From bc6953bdd284142ee93d13669f2216b2c8b0d7ad Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 18:43:28 +0200 Subject: [PATCH 39/46] fix(diff): drop the phantom trailing context line from parseFileDiff A raw git diff always ends with a newline, and indexDiffFiles joins per-file slices with "\n" too, so parseFileDiff's split("\n") yielded one trailing "" element on the last file's slice. That element was classified as a (deliberately, per AGENTS.md) blank context line, which landed a phantom zero-length context row on the last hunk of the last file and pushed its line counters past what the hunk header declared. This PR's own doc comment had started asserting that as deliberately correct, contradicting the Rust parser's prior explicit fix for the identical bug class (src-tauri/src/commands/read.rs). Drop exactly one trailing "" element (only when it is last) before any line is classified. A genuine blank context line mid-hunk, or one right before EOF in a diff ending in "\n\n", is untouched -- only the split artifact from the diff's own trailing newline is removed. Regression sweep: ran usePrPanel-lazy-diff.test.ts, usePrPanel-findings-render.test.ts, usePrPanel-lineAnnotations.test.ts, usePrPanel.test.ts, useReviewIntelligence.test.ts, DiffViewer-findings.test.ts, and PrInlineDiff.test.ts -- all pass unmodified, so no existing PR-review expectation encoded the phantom line as expected output. PullRequestPanel.vue carries its own private, byte-identical copy of this parser with the same defect; left untouched here (separate refactor, own regression surface), noted as a follow-up in the plan. --- .../src/utils/__tests__/unifiedDiff.test.ts | 94 +++++++++++++++++++ apps/desktop/src/utils/unifiedDiff.ts | 22 ++++- 2 files changed, 113 insertions(+), 3 deletions(-) diff --git a/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts b/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts index 50f6ca95..2955142d 100644 --- a/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts +++ b/apps/desktop/src/utils/__tests__/unifiedDiff.test.ts @@ -66,4 +66,98 @@ describe("parseFileDiff", () => { const blank = parsed.hunks[0].lines.find((l) => l.content === "" && l.type !== undefined); expect(blank?.type).toBe("context"); }); + + // v3.7.0 review-round fix (finding #10) — a raw `git diff` always ends + // with "\n" (indexDiffFiles joins slices with "\n" too), so + // `split("\n")` used to yield one trailing "" element that landed on the + // LAST hunk of the LAST file as a phantom zero-length context line, + // pushing its line counters past what the hunk header declared. Same bug + // class the Rust parser fixed (src-tauri/src/commands/read.rs). + describe("phantom trailing context line (finding #10)", () => { + it("a single-file diff ending in \\n has no trailing phantom context line, and its last real line's counters match the hunk header", () => { + const diff = [ + "diff --git a/a.ts b/a.ts", + "index 111..222 100644", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1,2 +1,2 @@", + " context a", + "-old a", + "+new a", + "", // trailing "\n" via join, below + ].join("\n"); + + const parsed = parseFileDiff(diff); + const lastHunk = parsed.hunks[parsed.hunks.length - 1]; + const lastLine = lastHunk.lines[lastHunk.lines.length - 1]; + + expect(lastLine).not.toEqual(expect.objectContaining({ type: "context", content: "" })); + // Hunk header says "+1,2" — 2 new lines declared (context a, new a). + // The last real line (the "+new a" add) must land at newLine 2, not 3. + expect(lastLine.type).toBe("add"); + expect(lastLine.newLineNo).toBe(2); + }); + + it("a multi-file diff ending in \\n only ever affected the LAST file's last hunk", () => { + const diff = THREE_FILE_DIFF + "\n"; + const slices = indexDiffFiles(diff); + expect(slices).toHaveLength(3); + + for (const slice of slices) { + const parsed = parseFileDiff(slice.raw); + const lastHunk = parsed.hunks[parsed.hunks.length - 1]; + const lastLine = lastHunk.lines[lastHunk.lines.length - 1]; + expect(lastLine).not.toEqual(expect.objectContaining({ type: "context", content: "" })); + } + }); + + it("the existing blank-context-line case (THREE_FILE_DIFF's b.ts) still classifies the interior \"\" as context, unmodified", () => { + const slice = indexDiffFiles(THREE_FILE_DIFF)[1]; + const parsed = parseFileDiff(slice.raw); + const blank = parsed.hunks[0].lines.find((l) => l.content === "" && l.type !== undefined); + expect(blank?.type).toBe("context"); + }); + + it("a diff ending in \\n\\n strips exactly one trailing element, so the genuine blank line before EOF survives as context", () => { + const diff = [ + "diff --git a/a.ts b/a.ts", + "index 111..222 100644", + "--- a/a.ts", + "+++ b/a.ts", + "@@ -1,2 +1,2 @@", + " context a", + "", + "-old a", + "+new a", + "", + "", + ].join("\n"); + + const parsed = parseFileDiff(diff); + const lastHunk = parsed.hunks[parsed.hunks.length - 1]; + const lastLine = lastHunk.lines[lastHunk.lines.length - 1]; + // The genuine blank context line right before EOF must survive. + expect(lastLine.type).toBe("context"); + expect(lastLine.content).toBe(""); + }); + + it("a diff NOT ending in a newline produces identical output to before (no line lost)", () => { + // THREE_FILE_DIFF itself has no trailing "\n" — regression guard that + // the fix does not drop a real line when there is no trailing "\n". + const parsed = parseUnifiedDiff(THREE_FILE_DIFF); + const cFile = parsed[parsed.length - 1]; + const lastHunk = cFile.hunks[cFile.hunks.length - 1]; + const lastLine = lastHunk.lines[lastHunk.lines.length - 1]; + expect(lastLine.type).toBe("add"); + expect(lastLine.content).toBe("new c"); + }); + + it("parseFileDiff still matches parseUnifiedDiff per file when the raw diff ends in \\n", () => { + const diff = THREE_FILE_DIFF + "\n"; + const expected = parseUnifiedDiff(diff); + const slices = indexDiffFiles(diff); + const actual = slices.map((s) => parseFileDiff(s.raw)); + expect(actual).toEqual(expected); + }); + }); }); diff --git a/apps/desktop/src/utils/unifiedDiff.ts b/apps/desktop/src/utils/unifiedDiff.ts index 85acbf83..d0515066 100644 --- a/apps/desktop/src/utils/unifiedDiff.ts +++ b/apps/desktop/src/utils/unifiedDiff.ts @@ -40,13 +40,29 @@ export function indexDiffFiles(rawDiff: string): { path: string; raw: string }[] /** Parse one file's raw `diff --git …` slice (as produced by `indexDiffFiles`) * into hunks/lines. Diff-parsing gotcha (AGENTS.md): context lines are - * detected via `line.startsWith(' ')` — a bare empty string is also treated - * as a (whitespace-stripped) context line, never as a phantom add/delete. */ + * detected via `line.startsWith(' ')` — a bare empty string mid-hunk is + * also treated as a (whitespace-stripped) context line, never as a phantom + * add/delete. A raw diff always ends with a newline (and `indexDiffFiles` + * joins slices with `"\n"` too), so `split("\n")` yields exactly one + * trailing `""` element that is not a diff line at all — v3.7.0 review-round + * fix (finding #10) drops exactly that trailing element (see below) before + * any line is classified, so it can never be mistaken for a genuine blank + * context line and pushed onto the last hunk as a phantom zero-length row. */ export function parseFileDiff(rawFileSlice: string): GitDiff { const file: GitDiff = { path: "unknown", hunks: [] }; let currentHunk: DiffHunk | null = null; let oldLine = 0, newLine = 0; - for (const line of rawFileSlice.split("\n")) { + const lines = rawFileSlice.split("\n"); + // A raw diff ends with a newline, so `split("\n")` yields one trailing "" + // element that is not a diff line at all. Dropping exactly that element + // (and only when it is last) keeps a genuine blank context line mid-hunk + // intact, while removing the phantom zero-length context row that + // otherwise lands on the last hunk of the last file and pushes its line + // counters past the hunk header. Same bug class the Rust parser fixed in + // src-tauri/src/commands/read.rs (a phantom trailing context line makes + // `git apply` reject the patch). + if (lines.length > 0 && lines[lines.length - 1] === "") lines.pop(); + for (const line of lines) { if (line.startsWith("diff --git ")) { const match = line.match(/diff --git a\/(.+) b\/(.+)/); file.path = match ? match[2] : "unknown"; From a6e445c2ca7bb830a2974ea5b2d49f4546ad5cc3 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 18:53:03 +0200 Subject: [PATCH 40/46] feat(a11y): trap and restore focus in BaseModal BaseModal had no focus trap, no initial focus, and no focus restore: only a window-level Escape handler. This is the shipped foundation for ~30 modals across the app (SecretsFindingsModal, CommitReviewModal, SettingsPanel, EditCommitOverlay, SplitCommitModal, and every other BaseModal consumer), a pre-existing gap this PR did not introduce. Add trapFocus/autoFocus props (both default true, both an escape hatch for a modal that needs different behavior). On mount, synchronously (not nextTick) remember the previously focused element and focus the panel; synchronous is load-bearing because Vue fires a child's mounted before its parent's, and AiTaskNameModal/CloneModal/ForkModal/ FolderPicker self-focus an input from their own onMounted + nextTick, so a nextTick here would race them. On unmount, restore focus to the remembered element if it is still in the document. A bubble-phase keydown listener on the panel itself (not window) traps Tab, wrapping at the ends via the new pure utils/focusTrap.ts (focusableWithin + nextTrapTarget, unit tested without mounting a component), and bails on e.defaultPrevented so an inner component that owns Tab (CodeMirror, xterm) always wins first. Manually reasoned through (no live browser available in this session, recommend a dev:web pass before merge): - CloneModal, AiTaskNameModal, ForkModal: their nextTick self-focus still wins, confirmed by the synchronous-vs-nextTick ordering. - FolderPicker: its own separate overlay (not a BaseModal instance), rendered as a sibling in App.vue, not nested inside CloneModal's panel DOM -- no double-trap interaction. - EditCommitOverlay: NOT in the plan's originally-identified list of self-focusing modals -- it self-focuses via watch(entry, ..., {immediate:true}) + setTimeout(50), not onMounted + nextTick. Verified this still ends up correct: the 50ms macrotask reliably fires after BaseModal's synchronous focus and after any nextTick, so the summary textarea still wins the race. Flagging this as a plan-completeness gap, not a functional regression. - No modal in this codebase currently embeds CodeMirror or xterm inside a BaseModal (TerminalPanel/FileExplorerPanel are docked panels, MergeEditor is a full view) -- the e.defaultPrevented guard is a forward-looking safeguard, not exercised by any existing modal today. - askConfirm's generic confirm modal is likewise a sibling BaseModal instance at App.vue's root, not DOM-nested inside another modal's panel -- each panel's own bubble-phase listener only reacts when focus is within its own subtree. Full desktop suite (107 files / 937 tests) run and green after this commit specifically, not just at the end of the plan. --- apps/desktop/src/components/BaseModal.vue | 83 ++++++++- .../__tests__/BaseModal.focus.test.ts | 168 ++++++++++++++++++ .../src/utils/__tests__/focusTrap.test.ts | 113 ++++++++++++ apps/desktop/src/utils/focusTrap.ts | 77 ++++++++ 4 files changed, 440 insertions(+), 1 deletion(-) create mode 100644 apps/desktop/src/components/__tests__/BaseModal.focus.test.ts create mode 100644 apps/desktop/src/utils/__tests__/focusTrap.test.ts create mode 100644 apps/desktop/src/utils/focusTrap.ts diff --git a/apps/desktop/src/components/BaseModal.vue b/apps/desktop/src/components/BaseModal.vue index 317181ef..bd958fd1 100644 --- a/apps/desktop/src/components/BaseModal.vue +++ b/apps/desktop/src/components/BaseModal.vue @@ -1,6 +1,7 @@ @@ -87,11 +159,14 @@ onUnmounted(() => { @click.self="onBackdropClick" >
@@ -180,6 +255,12 @@ onUnmounted(() => { animation: bm-slide-in var(--transition-slow) ease; } +/* v3.7.0 review-round fix (finding #11) — the panel is `tabindex="-1"` so it + is programmatically focusable (initial focus, empty-focusable-set fallback); + suppress the focus ring that would otherwise draw on the whole panel. + A single class + pseudo-class — does not touch `.bm-btn` (AGENTS.md). */ +.base-modal:focus { outline: none; } + .base-modal--sm { width: min(400px, 92vw); } .base-modal--md { width: min(520px, 92vw); } .base-modal--lg { width: min(640px, 92vw); } diff --git a/apps/desktop/src/components/__tests__/BaseModal.focus.test.ts b/apps/desktop/src/components/__tests__/BaseModal.focus.test.ts new file mode 100644 index 00000000..46d5149c --- /dev/null +++ b/apps/desktop/src/components/__tests__/BaseModal.focus.test.ts @@ -0,0 +1,168 @@ +/** + * v3.7.0 review-round fix (finding #11) — `BaseModal` focus trap, initial + * focus, and focus restore. Own commit, extra scrutiny: this changes + * behavior for every modal in the app. Mounted via `h()` so the default + * slot can carry real focusable content (two buttons), mirroring the + * `createApp` convention used by `CommitReviewModal.test.ts` / + * `SecretsFindingsModal.test.ts`. + */ +import { describe, it, expect, vi, afterEach } from "vitest"; +import { createApp, h, type App } from "vue"; +import BaseModal from "../BaseModal.vue"; + +let app: App | null = null; +let container: HTMLElement; +let outsideButton: HTMLButtonElement; + +function mount(props: Record = {}, slotContent?: () => unknown) { + container = document.createElement("div"); + document.body.appendChild(container); + app = createApp({ + render() { + return h(BaseModal, props, slotContent ? { default: slotContent } : undefined); + }, + }); + app.mount(container); +} + +/** An element outside the modal, focused before mount, to test focus restore. */ +function focusOutsideElement() { + outsideButton = document.createElement("button"); + outsideButton.id = "outside-btn"; + document.body.appendChild(outsideButton); + outsideButton.focus(); +} + +afterEach(() => { + app?.unmount(); + app = null; + container?.remove(); + outsideButton?.remove(); +}); + +function defaultSlotButtons() { + return [ + h("button", { class: "first-btn" }, "First"), + h("button", { class: "last-btn" }, "Last"), + ]; +} + +describe("BaseModal — focus trap (finding #11)", () => { + it("focuses the panel on mount by default", async () => { + focusOutsideElement(); + mount({ title: "T" }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + const panel = document.querySelector(".base-modal"); + expect(document.activeElement).toBe(panel); + }); + + it("does not change focus on mount when autoFocus is false", async () => { + focusOutsideElement(); + mount({ title: "T", autoFocus: false }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + expect(document.activeElement).toBe(outsideButton); + }); + + it("restores focus to the previously focused element on unmount", async () => { + focusOutsideElement(); + mount({ title: "T" }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + app?.unmount(); + app = null; + + expect(document.activeElement).toBe(outsideButton); + }); + + it("Tab from the last focusable wraps to the first, with preventDefault called", async () => { + // hideHeader avoids the header's own close button joining the + // focusable set, so "first"/"last" unambiguously refer to the two + // slot buttons. + mount({ title: "T", hideHeader: true }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + const panel = document.querySelector(".base-modal")!; + const first = document.querySelector(".first-btn")!; + const last = document.querySelector(".last-btn")!; + last.focus(); + + const event = new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true }); + const preventDefaultSpy = vi.spyOn(event, "preventDefault"); + panel.dispatchEvent(event); + + expect(preventDefaultSpy).toHaveBeenCalled(); + expect(document.activeElement).toBe(first); + }); + + it("Shift+Tab from the first focusable wraps to the last, with preventDefault called", async () => { + mount({ title: "T", hideHeader: true }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + const panel = document.querySelector(".base-modal")!; + const first = document.querySelector(".first-btn")!; + const last = document.querySelector(".last-btn")!; + first.focus(); + + const event = new KeyboardEvent("keydown", { key: "Tab", shiftKey: true, bubbles: true, cancelable: true }); + const preventDefaultSpy = vi.spyOn(event, "preventDefault"); + panel.dispatchEvent(event); + + expect(preventDefaultSpy).toHaveBeenCalled(); + expect(document.activeElement).toBe(last); + }); + + it("a Tab event with defaultPrevented already true is ignored (the CodeMirror/xterm guard)", async () => { + mount({ title: "T" }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + const panel = document.querySelector(".base-modal")!; + const first = document.querySelector(".first-btn")!; + const last = document.querySelector(".last-btn")!; + last.focus(); + + const event = new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true }); + event.preventDefault(); // simulates an inner component (CodeMirror/xterm) already handling Tab + panel.dispatchEvent(event); + + // The trap must not additionally move focus — `last` stays focused. + expect(document.activeElement).toBe(last); + void first; + }); + + it("trapFocus: false disables the wrap entirely", async () => { + mount({ title: "T", trapFocus: false }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + const panel = document.querySelector(".base-modal")!; + const last = document.querySelector(".last-btn")!; + last.focus(); + + const event = new KeyboardEvent("keydown", { key: "Tab", bubbles: true, cancelable: true }); + const preventDefaultSpy = vi.spyOn(event, "preventDefault"); + panel.dispatchEvent(event); + + expect(preventDefaultSpy).not.toHaveBeenCalled(); + expect(document.activeElement).toBe(last); + }); + + it("Escape still emits close, and closable: false still suppresses it (regression guard)", async () => { + const onClose = vi.fn(); + mount({ title: "T", onClose }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + + window.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" })); + expect(onClose).toHaveBeenCalledTimes(1); + + app?.unmount(); + app = null; + container?.remove(); + + const onCloseNonClosable = vi.fn(); + mount({ title: "T", closable: false, onClose: onCloseNonClosable }, defaultSlotButtons); + await new Promise((r) => setTimeout(r, 0)); + window.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape" })); + expect(onCloseNonClosable).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/desktop/src/utils/__tests__/focusTrap.test.ts b/apps/desktop/src/utils/__tests__/focusTrap.test.ts new file mode 100644 index 00000000..10f454e0 --- /dev/null +++ b/apps/desktop/src/utils/__tests__/focusTrap.test.ts @@ -0,0 +1,113 @@ +/** + * v3.7.0 review-round fix (finding #11) — pure focus-trap arithmetic, unit + * tested without mounting a component. `BaseModal.vue` stays thin: it just + * calls `focusableWithin`/`nextTrapTarget` from its `Tab` handler. + */ +import { describe, it, expect } from "vitest"; +import { focusableWithin, nextTrapTarget } from "../focusTrap"; + +function makeRoot(html: string): HTMLElement { + const root = document.createElement("div"); + root.innerHTML = html; + document.body.appendChild(root); + return root; +} + +describe("focusableWithin", () => { + it("finds buttons, links-with-href, inputs, selects, textareas, and [tabindex='0'] in DOM order", () => { + const root = makeRoot(` + + link + + + +
focusable div
+ `); + const found = focusableWithin(root).map((el) => el.id); + expect(found).toEqual(["b1", "a1", "i1", "s1", "t1", "d1"]); + root.remove(); + }); + + it("excludes disabled elements", () => { + const root = makeRoot(``); + expect(focusableWithin(root).map((el) => el.id)).toEqual(["b1"]); + root.remove(); + }); + + it("excludes tabindex='-1' elements", () => { + const root = makeRoot(``); + expect(focusableWithin(root).map((el) => el.id)).toEqual(["b1"]); + root.remove(); + }); + + it("excludes hidden elements", () => { + const root = makeRoot(``); + expect(focusableWithin(root).map((el) => el.id)).toEqual(["b1"]); + root.remove(); + }); + + it("excludes aria-hidden='true' elements", () => { + const root = makeRoot(``); + expect(focusableWithin(root).map((el) => el.id)).toEqual(["b1"]); + root.remove(); + }); + + it("excludes an a without href", () => { + const root = makeRoot(`no href`); + expect(focusableWithin(root).map((el) => el.id)).toEqual(["b1"]); + root.remove(); + }); + + it("returns [] for a root with no focusable descendants", () => { + const root = makeRoot(`
just text
`); + expect(focusableWithin(root)).toEqual([]); + root.remove(); + }); +}); + +describe("nextTrapTarget", () => { + it("wraps from the last focusable to the first when moving forward", () => { + const root = makeRoot(``); + const focusables = focusableWithin(root); + const last = focusables[focusables.length - 1]; + expect(nextTrapTarget(focusables, last, false)).toBe(focusables[0]); + root.remove(); + }); + + it("wraps from the first focusable to the last when moving backward (Shift+Tab)", () => { + const root = makeRoot(``); + const focusables = focusableWithin(root); + const first = focusables[0]; + expect(nextTrapTarget(focusables, first, true)).toBe(focusables[focusables.length - 1]); + root.remove(); + }); + + it("returns the first element when the active element is outside the focusable set (forward)", () => { + const root = makeRoot(``); + const focusables = focusableWithin(root); + const outside = document.createElement("div"); + expect(nextTrapTarget(focusables, outside, false)).toBe(focusables[0]); + root.remove(); + }); + + it("returns the last element when the active element is outside the focusable set (backward)", () => { + const root = makeRoot(``); + const focusables = focusableWithin(root); + const outside = document.createElement("div"); + expect(nextTrapTarget(focusables, outside, true)).toBe(focusables[focusables.length - 1]); + root.remove(); + }); + + it("returns null for an empty focusable set", () => { + expect(nextTrapTarget([], null, false)).toBeNull(); + expect(nextTrapTarget([], null, true)).toBeNull(); + }); + + it("returns null for a middle element (not a wrap boundary) — the browser's default Tab handles it, the trap must not interfere", () => { + const root = makeRoot(``); + const focusables = focusableWithin(root); + expect(nextTrapTarget(focusables, focusables[1], false)).toBeNull(); + expect(nextTrapTarget(focusables, focusables[1], true)).toBeNull(); + root.remove(); + }); +}); diff --git a/apps/desktop/src/utils/focusTrap.ts b/apps/desktop/src/utils/focusTrap.ts new file mode 100644 index 00000000..a8c03281 --- /dev/null +++ b/apps/desktop/src/utils/focusTrap.ts @@ -0,0 +1,77 @@ +/** + * focusTrap.ts + * + * v3.7.0 review-round fix (finding #11) — pure focus-trap arithmetic for + * `BaseModal.vue`. No Vue, no DOM assumptions beyond the standard DOM API, + * so it is unit-testable without mounting a component. `BaseModal` stays + * thin: it just calls `focusableWithin`/`nextTrapTarget` from its own `Tab` + * keydown handler. + */ + +const FOCUSABLE_SELECTOR = [ + "button", + "a[href]", + "input", + "select", + "textarea", + '[tabindex="0"]', +].join(", "); + +/** + * Every visible, non-disabled, focusable element inside `root`, in DOM + * order. Filters out `disabled`, `tabindex="-1"`, `hidden`, and + * `aria-hidden="true"` elements. + * + * A zero-size (`display: none` via CSS, not the `hidden` attribute) element + * would ideally also be excluded, but `offsetWidth`/`offsetHeight` are + * always `0` under jsdom (no real layout engine), so a strict size check + * would make every element in the unit-test suite look "invisible" and + * break this function for its only automated test harness. The manual + * `dev:web` QA pass (see the v3.7.0 review-round fix plan, Task 11) is what + * actually exercises this in a real layout engine; real CSS-hidden content + * inside a modal is rare in this codebase (everything conditionally shown + * uses `v-if`, which removes the element from the DOM entirely — so this + * selector never sees it). + */ +export function focusableWithin(root: HTMLElement): HTMLElement[] { + const candidates = Array.from(root.querySelectorAll(FOCUSABLE_SELECTOR)); + return candidates.filter((el) => { + if (el.hasAttribute("disabled")) return false; + if (el.getAttribute("tabindex") === "-1") return false; + if (el.hasAttribute("hidden")) return false; + if (el.getAttribute("aria-hidden") === "true") return false; + return true; + }); +} + +/** + * Computes the wrap target for a `Tab` press inside a focus trap. Returns + * `null` when no wrap is needed — the browser's own default Tab behavior + * moves focus correctly for anything that is not a wrap boundary, and the + * caller must not call `preventDefault()` in that case. + * + * - Forward (`backwards: false`): wraps to the FIRST focusable when `active` + * is the last one, or is not found in `focusables` at all (outside the + * trapped set — e.g. focus was on the backdrop). + * - Backward (`backwards: true`, Shift+Tab): wraps to the LAST focusable + * when `active` is the first one, or is outside the set. + * - Empty focusable set: always `null` (the caller falls back to focusing + * the panel itself). + */ +export function nextTrapTarget( + focusables: HTMLElement[], + active: Element | null, + backwards: boolean, +): HTMLElement | null { + if (focusables.length === 0) return null; + + const index = active ? focusables.indexOf(active as HTMLElement) : -1; + + if (backwards) { + if (index <= 0) return focusables[focusables.length - 1]; + return null; + } + + if (index === -1 || index === focusables.length - 1) return focusables[0]; + return null; +} From 612207412e902caf8bf8a0f64abe7084b7e9620e Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 18:55:21 +0200 Subject: [PATCH 41/46] fix(folder-picker): select the typed path without requiring Enter first "Select this folder" called emit("select", currentPath.value), and currentPath is only ever written by fetchDir (reached via navigate/goUp/goHome/onInputEnter) -- so a path typed into the input was silently ignored unless Enter was pressed first. selectCurrent() now resolves the typed path through fetchDir first when it differs from currentPath, normalizing it and surfacing a bad path as this dialog's own inline error instead of failing downstream in openRepo. fetchDir is already non-throwing and sets errorMsg, so no new error handling is needed. --- apps/desktop/src/components/FolderPicker.vue | 12 +- .../components/__tests__/FolderPicker.test.ts | 150 ++++++++++++++++++ 2 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 apps/desktop/src/components/__tests__/FolderPicker.test.ts diff --git a/apps/desktop/src/components/FolderPicker.vue b/apps/desktop/src/components/FolderPicker.vue index 09951503..c8ea413f 100644 --- a/apps/desktop/src/components/FolderPicker.vue +++ b/apps/desktop/src/components/FolderPicker.vue @@ -53,7 +53,17 @@ function onInputEnter() { if (val) fetchDir(val); } -function selectCurrent() { +async function selectCurrent() { + const typed = pathInput.value.trim(); + // v3.7.0 review-round fix (finding #12) — "Select this folder" must honor + // whatever is in the input, not only a path the user already navigated to + // with Enter. Resolving it through fetchDir first normalizes it (trailing + // slash, relative segments, home expansion) and surfaces a bad path as + // this dialog's own error instead of failing downstream in openRepo. + if (typed && typed !== currentPath.value) { + await fetchDir(typed); + if (errorMsg.value) return; // fetchDir sets errorMsg and never throws + } emit("select", currentPath.value); } diff --git a/apps/desktop/src/components/__tests__/FolderPicker.test.ts b/apps/desktop/src/components/__tests__/FolderPicker.test.ts new file mode 100644 index 00000000..f7b1773e --- /dev/null +++ b/apps/desktop/src/components/__tests__/FolderPicker.test.ts @@ -0,0 +1,150 @@ +/** + * v3.7.0 review-round fix (finding #12) — `FolderPicker`'s "Select this + * folder" button silently ignored whatever was typed into the path input + * unless Enter had already been pressed first: it emitted `select` with + * `currentPath`, which is only ever written by `fetchDir` (reached via + * navigate/goUp/goHome/onInputEnter), never by typing alone. + * + * Mocks `../../utils/backend` (`listDir`) — jsdom `localStorage` is patched + * by `src/test-setup.ts`, so `useFolderHistory` works as-is, matching the + * established convention (`CommitReviewModal.test.ts`, `createApp` mount). + */ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { createApp, type App } from "vue"; +import FolderPicker from "../FolderPicker.vue"; + +const listDirMock = vi.fn(); + +vi.mock("../../utils/backend", () => ({ + listDir: (...a: unknown[]) => listDirMock(...a), +})); + +let app: App | null = null; +let container: HTMLElement; + +function mount(props: Record = {}) { + container = document.createElement("div"); + document.body.appendChild(container); + app = createApp(FolderPicker, props); + app.mount(container); +} + +function dirResult(current: string, overrides: Partial<{ parent: string | null; home: string; dirs: unknown[] }> = {}) { + return { + current, + parent: overrides.parent ?? null, + home: overrides.home ?? "/home/user", + dirs: overrides.dirs ?? [], + }; +} + +beforeEach(() => { + localStorage.clear(); + listDirMock.mockReset(); + // Initial onMounted fetchDir() call (home dir, no args). + listDirMock.mockResolvedValue(dirResult("/home/user")); +}); + +afterEach(() => { + app?.unmount(); + app = null; + container?.remove(); +}); + +async function flush() { + await new Promise((r) => setTimeout(r, 0)); + await new Promise((r) => setTimeout(r, 0)); +} + +describe("FolderPicker — select the typed path without requiring Enter (finding #12)", () => { + it("typing a path and clicking Select (no Enter) resolves it via listDir and emits the resolved path", async () => { + const onSelect = vi.fn(); + mount({ onSelect }); + await flush(); + + listDirMock.mockResolvedValueOnce(dirResult("/typed/resolved/path")); + + const input = document.querySelector(".fp-path-input")!; + input.value = "/typed/path"; + input.dispatchEvent(new Event("input")); + await flush(); + + const selectBtn = document.querySelector(".fp-btn--select")!; + selectBtn.click(); + await flush(); + + expect(listDirMock).toHaveBeenCalledWith("/typed/path"); + expect(onSelect).toHaveBeenCalledWith("/typed/resolved/path"); + }); + + it("listDir rejecting for the typed path emits no select and renders the error", async () => { + const onSelect = vi.fn(); + mount({ onSelect }); + await flush(); + + listDirMock.mockRejectedValueOnce(new Error("no such directory")); + + const input = document.querySelector(".fp-path-input")!; + input.value = "/bad/path"; + input.dispatchEvent(new Event("input")); + await flush(); + + const selectBtn = document.querySelector(".fp-btn--select")!; + selectBtn.click(); + await flush(); + + expect(onSelect).not.toHaveBeenCalled(); + expect(document.body.textContent).toContain("no such directory"); + }); + + it("typing nothing and clicking Select emits the current path, with no redundant listDir call", async () => { + const onSelect = vi.fn(); + mount({ onSelect }); + await flush(); + + const callsBeforeClick = listDirMock.mock.calls.length; + + const selectBtn = document.querySelector(".fp-btn--select")!; + selectBtn.click(); + await flush(); + + expect(listDirMock.mock.calls.length).toBe(callsBeforeClick); // no new call + expect(onSelect).toHaveBeenCalledWith("/home/user"); + }); + + it("typing a path identical to currentPath causes no redundant listDir call, and still emits", async () => { + const onSelect = vi.fn(); + mount({ onSelect }); + await flush(); + + const input = document.querySelector(".fp-path-input")!; + input.value = "/home/user"; // same as currentPath already + input.dispatchEvent(new Event("input")); + await flush(); + + const callsBeforeClick = listDirMock.mock.calls.length; + const selectBtn = document.querySelector(".fp-btn--select")!; + selectBtn.click(); + await flush(); + + expect(listDirMock.mock.calls.length).toBe(callsBeforeClick); + expect(onSelect).toHaveBeenCalledWith("/home/user"); + }); + + it("pressing Enter in the input still navigates (regression guard)", async () => { + mount({}); + await flush(); + + listDirMock.mockResolvedValueOnce(dirResult("/navigated/path")); + + const input = document.querySelector(".fp-path-input")!; + input.value = "/some/path"; + input.dispatchEvent(new Event("input")); + input.dispatchEvent(new KeyboardEvent("keydown", { key: "Enter" })); + await flush(); + + expect(listDirMock).toHaveBeenCalledWith("/some/path"); + // currentPath is now updated to what fetchDir resolved. + expect(input.value).toBe("/navigated/path"); + }); +}); From 94a6cf42ed3deee60921d5ff345f73ffd76cb8d7 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 18:56:52 +0200 Subject: [PATCH 42/46] fix(settings): state that the AI toggle also gates the review features The "Enable AI suggestions" checkbox and its hint were conflict-only, so a user who only wanted Commit Review (or PR pre-review) had no reason to tick it and never discovered those features' settings exist -- everything below the toggle, including the Commit Review group, is gated on it. Copy-only fix, per decision O4: reword the label/hint in all 5 locales so the toggle states its full scope (conflict-resolution suggestions, commit messages, PR pre-review, Commit Review). No template, settings, or behavior change -- ungating commitReviewEnabled from aiEnabled would let the toggle be on while the provider is unavailable, showing a "Review staged changes" button that silently does nothing, a worse UX than the current discoverability gap. No new test: no key was added or removed (pnpm build's vue-tsc check is the only verification a pure value change needs), and there is no existing test asserting copy values to update. --- apps/desktop/src/locales/en.ts | 4 ++-- apps/desktop/src/locales/es.ts | 4 ++-- apps/desktop/src/locales/fr.ts | 4 ++-- apps/desktop/src/locales/pt-BR.ts | 4 ++-- apps/desktop/src/locales/zh-CN.ts | 4 ++-- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/apps/desktop/src/locales/en.ts b/apps/desktop/src/locales/en.ts index 136438bd..0f00a44e 100644 --- a/apps/desktop/src/locales/en.ts +++ b/apps/desktop/src/locales/en.ts @@ -1086,8 +1086,8 @@ const en = { logsLevelWarn: "WARN", logsLevelInfo: "INFO", // AI tab (header block) - aiEnable: "Enable AI suggestions", - aiEnableHint: "Proposes smart resolutions for complex conflicts (confidence < 60%)", + aiEnable: "Enable AI features", + aiEnableHint: "Turns on every AI-powered feature: conflict-resolution suggestions, commit messages, PR pre-review, and Commit Review of your staged changes. Nothing runs until a provider below is configured.", aiPrivacyNote: "The AI analyzes the conflict context (base/ours/theirs, commit messages, file name) to propose a resolution. Your code is only sent to the selected provider. No suggestion is applied automatically.", // AI tab — provider selector aiProviderLabel: "Provider", diff --git a/apps/desktop/src/locales/es.ts b/apps/desktop/src/locales/es.ts index bfff0160..fef33e61 100644 --- a/apps/desktop/src/locales/es.ts +++ b/apps/desktop/src/locales/es.ts @@ -1068,8 +1068,8 @@ const es: Locale = { logsLevelError: "ERROR", logsLevelWarn: "AVISO", logsLevelInfo: "INFO", - aiEnable: "Activar sugerencias de IA", - aiEnableHint: "Propone resoluciones inteligentes para conflictos complejos (confianza < 60%)", + aiEnable: "Activar las funciones de IA", + aiEnableHint: "Activa todas las funciones basadas en IA: sugerencias de resolución de conflictos, mensajes de commit, pre-revisión de PR y Revisión de commit de tus cambios en stage. Nada se ejecuta hasta que configures un proveedor abajo.", aiPrivacyNote: "La IA analiza el contexto del conflicto (base/ours/theirs, mensajes de commit, nombre de archivo) para proponer una resolución. Tu código solo se envía al proveedor seleccionado. Ninguna sugerencia se aplica automáticamente.", aiProviderLabel: "Proveedor", aiProviderClaude: "Claude (API de Anthropic)", diff --git a/apps/desktop/src/locales/fr.ts b/apps/desktop/src/locales/fr.ts index 42b25e89..e3866ee0 100644 --- a/apps/desktop/src/locales/fr.ts +++ b/apps/desktop/src/locales/fr.ts @@ -1077,8 +1077,8 @@ const fr: Locale = { logsLevelError: "ERREUR", logsLevelWarn: "AVERT", logsLevelInfo: "INFO", - aiEnable: "Activer les suggestions IA", - aiEnableHint: "Propose des r\u00e9solutions intelligentes pour les conflits complexes (confiance < 60%)", + aiEnable: "Activer les fonctionnalit\u00e9s IA", + aiEnableHint: "Active toutes les fonctionnalit\u00e9s bas\u00e9es sur l'IA : suggestions de r\u00e9solution de conflits, messages de commit, pr\u00e9-revue de PR, et Revue de commit de vos changements index\u00e9s. Rien ne s'ex\u00e9cute tant qu'un fournisseur n'est pas configur\u00e9 ci-dessous.", aiPrivacyNote: "L'IA analyse le contexte du conflit (base/ours/theirs, messages de commit, nom de fichier) pour proposer une r\u00e9solution. Votre code n'est envoy\u00e9 qu'au provider s\u00e9lectionn\u00e9. Aucune suggestion n'est appliqu\u00e9e automatiquement.", aiProviderLabel: "Provider", aiProviderClaude: "Claude (API Anthropic)", diff --git a/apps/desktop/src/locales/pt-BR.ts b/apps/desktop/src/locales/pt-BR.ts index aeb3b276..85e814b9 100644 --- a/apps/desktop/src/locales/pt-BR.ts +++ b/apps/desktop/src/locales/pt-BR.ts @@ -1069,8 +1069,8 @@ const ptBR: Locale = { logsLevelError: "ERRO", logsLevelWarn: "AVISO", logsLevelInfo: "INFO", - aiEnable: "Ativar sugestões de IA", - aiEnableHint: "Propõe resoluções inteligentes para conflitos complexos (confiança < 60%)", + aiEnable: "Ativar recursos de IA", + aiEnableHint: "Ativa todos os recursos baseados em IA: sugestões de resolução de conflitos, mensagens de commit, pré-revisão de PR e Revisão de commit das suas mudanças em stage. Nada é executado até que um provedor seja configurado abaixo.", aiPrivacyNote: "A IA analisa o contexto do conflito (base/ours/theirs, mensagens de commit, nome do arquivo) para propor uma resolução. Seu código é enviado apenas ao provedor selecionado. Nenhuma sugestão é aplicada automaticamente.", aiProviderLabel: "Provedor", aiProviderClaude: "Claude (API da Anthropic)", diff --git a/apps/desktop/src/locales/zh-CN.ts b/apps/desktop/src/locales/zh-CN.ts index 4871cffb..4ab85df9 100644 --- a/apps/desktop/src/locales/zh-CN.ts +++ b/apps/desktop/src/locales/zh-CN.ts @@ -1127,8 +1127,8 @@ const zhCN: Locale = { logsLevelError: "错误", logsLevelWarn: "警告", logsLevelInfo: "信息", - aiEnable: "启用 AI 建议", - aiEnableHint: "为复杂冲突（置信度 < 60%）提出智能解决方案", + aiEnable: "启用 AI 功能", + aiEnableHint: "启用所有由 AI 驱动的功能：冲突解决建议、提交信息、PR 预审查，以及对暂存更改的提交审查。在下方配置好提供方之前，不会运行任何功能。", aiPrivacyNote: "AI 会分析冲突上下文（base/ours/theirs、提交信息、文件名）以提出解决方案。你的代码只会发送给你选择的服务商。任何建议都不会自动应用。", aiProviderLabel: "服务商", aiProviderClaude: "Claude（Anthropic API）", From 9a0b80e786c4ec7abf1cfcebf0d39fa502ddaae2 Mon Sep 17 00:00:00 2001 From: Laurent Guitton Date: Wed, 19 Aug 2026 19:01:06 +0200 Subject: [PATCH 43/46] fix(settings): translate the nav group headers and flag the local CLI fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 14a: the four Settings nav sidebar group headers were hardcoded strings (three of them French: "Dépôt", "IA & Agents", "Système"), rendered with no t() call, shown untranslated in every locale, while every sibling tab label already goes through t(). Changed settingsNavGroups to carry a labelKey instead of a raw label string, added 4 new keys (navGroupApplication/Repo/Ai/System) to all 5 locales. 14b: useAIProvider.ts intentionally falls through to the local Claude Code CLI when the selected provider is genuinely misconfigured (a confirmed, working-as-intended behavior, not a bug), but nothing in the Settings UI said so. Add one conditional hint line under the provider select, shown only when the selected provider (claude or openai-compat) is missing its required config AND the CLI is present (SettingsPanel already holds claudeCliInfo for this). Does not change which provider is picked. One new key (aiProviderCliFallbackHint) in all 5 locales. Both trivial, zero-risk, cheap i18n/indicator fixes bundled per decision O5; no new test (no test asserts copy values or nav-group rendering specifically), pnpm build's vue-tsc missing-key check is the verification. --- apps/desktop/src/components/SettingsPanel.vue | 32 +++++++++++++++---- apps/desktop/src/locales/en.ts | 7 ++++ apps/desktop/src/locales/es.ts | 7 ++++ apps/desktop/src/locales/fr.ts | 7 ++++ apps/desktop/src/locales/pt-BR.ts | 7 ++++ apps/desktop/src/locales/zh-CN.ts | 7 ++++ 6 files changed, 60 insertions(+), 7 deletions(-) diff --git a/apps/desktop/src/components/SettingsPanel.vue b/apps/desktop/src/components/SettingsPanel.vue index 21619ad9..90216643 100644 --- a/apps/desktop/src/components/SettingsPanel.vue +++ b/apps/desktop/src/components/SettingsPanel.vue @@ -1,6 +1,7 @@