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 d7ba20bd..6a6bbff3 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -71,6 +71,7 @@ const SplitCommitModal = defineAsyncComponent(() => import("./components/SplitCo const BranchDirtySwitchModal = defineAsyncComponent(() => import("./components/BranchDirtySwitchModal.vue")); const SecretsFindingsModal = defineAsyncComponent(() => import("./components/SecretsFindingsModal.vue")); const CommitReviewModal = defineAsyncComponent(() => import("./components/CommitReviewModal.vue")); +const CommitReviewDecisionModal = defineAsyncComponent(() => import("./components/CommitReviewDecisionModal.vue")); import { useStashMessage } from "./composables/useStashMessage"; import { useAIProvider } from "./composables/useAIProvider"; import { usePrPanel, PR_PANEL_KEY } from "./composables/usePrPanel"; @@ -98,6 +99,14 @@ import { useLaunchpadPoller } from "./composables/useLaunchpadPoller"; import { useSecretsScanner } from "./composables/useSecretsScanner"; import { useCommitReview } from "./composables/useCommitReview"; import { useCommitReviewNav } from "./composables/useCommitReviewNav"; +import { buildReviewFixPrompt } from "./utils/reviewFixPrompt"; +import { + buildReviewTrailer, + resolveCommitReviewGate, + effectiveReviewDecision, + appendReviewTrailer, + type ReviewDecision, +} from "./composables/commitReviewState"; import { resolveCommitReviewShortcut } from "./composables/commitReviewKeymap"; import { useLaunchpadPrs } from "./composables/useLaunchpadPrs"; import { diffLaunchpad, isBotAuthor, type LaunchpadEvent } from "./composables/useLaunchpadNotifications"; @@ -117,7 +126,7 @@ import { TOGGLE_GIT_TREE_KEY, OPEN_SETTINGS_KEY, } from "./composables/branchPickerBridge"; -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 { gitStash, gitStashPop, gitStashList, openInEditor, setGitConfig, gitDiscard, gitAddToGitignore, gitDeleteBranch, gitDeleteTag, gitDeleteRemoteTag, gitRemoteInfo, gitUnpushedTags, gitPushTags, gitMergeBase, gitResetToCommit, gitCommitSubmoduleChanges, gitSubmoduleCheckUpdates, scratchWorktreeCreate, scratchWorktreeDiscard, scratchWorktreeMergeBack, gitWorktreeList, gitWorktreeRemove, type CommitSubmoduleChange, type ScratchWorktree } from "./utils/backend"; import { useCommitActions } from "./composables/useCommitActions"; const { t, locale } = useI18n(); @@ -134,6 +143,14 @@ let lastAttemptedCommitTrailers = ""; // v3.7.0 — Commit Review (local, opt-in, off by default; see useCommitReview.ts). const commitReview = useCommitReview(); const showCommitReviewModal = ref(false); +// Task 5 — the Review/Vouch/Skip decision recorded for the CURRENT commit +// cycle. Reset on repo switch and after a successful commit (a new commit +// starts a new cycle) — see the watchers/handlers below. +const showCommitReviewDecisionModal = ref(false); +const commitReviewDecision = ref(null); +/** Trailers captured when the decision modal needs to open — re-applied once + * the user picks Vouch/Skip (Review now leaves the commit un-issued). */ +let pendingCommitReviewTrailers = ""; // `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 @@ -234,6 +251,7 @@ const { stagePatch, unstagePatch, commit: doCommit, + lastCommitHash, amendCommit: doAmendCommit, push: doPush, pull: doPull, @@ -1100,12 +1118,16 @@ const repoSidebarProps = computed(() => ({ commitReviewFindingsCount: commitReview.findings.value.length, commitReviewProgress: commitReview.progress.value, reviewFindingsByFile: commitReview.findingsByFile.value, + commitReviewIterations: commitReview.iterations.value, + commitReviewCoverage: commitReview.coverage.value, })); /** * Intercepts the commit action when the secrets scanner has active findings: shows a * non-blocking confirm (never a hard stop — the user can always proceed) before delegating to - * `doCommit`. Wired to both RepoSidebar's commit button and the findings modal's "Commit anyway". + * `proceedToCommit`. Wired to both RepoSidebar's commit button and the findings modal's + * "Commit anyway" (`onSecretsCommitAnyway`) — both funnel into the same post-secrets-gate path + * so the Commit Review decision gate (Task 5) applies consistently either way. */ async function handleCommitRequest(trailers: string) { lastAttemptedCommitTrailers = trailers; @@ -1118,7 +1140,128 @@ async function handleCommitRequest(trailers: string) { }); if (!confirmed) return; } - await doCommit(trailers); + await proceedToCommit(trailers); +} + +/** + * v3.7.0 (Task 5) — the single place that assembles the final trailer block + * and calls `doCommit`, after any secrets gate has already been cleared. + * `resolveCommitReviewGate`/`effectiveReviewDecision` (pure, unit-tested in + * `commitReviewState.test.ts`) decide whether the Review/Vouch/Skip decision + * modal needs to open first — never a hard stop; cancelling that modal + * cancels the commit (decision D8), it never silently records "skipped". + * + * The review trailer is appended HERE rather than round-tripped through a + * RepoSidebar prop: `RepoSidebar.buildTrailers()` runs and emits BEFORE this + * 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. + * + * Second verifier pass (HIGH) — `computeCurrentCoverage` is awaited right + * before `buildReviewTrailer`, recomputing coverage against the FULL + * current staged diff at the moment of commit. Two scenarios otherwise let + * a stale `coverage:100%` slip into the trailer: (B) editing and restaging + * a file that was already reviewed doesn't change the staged file COUNT, + * so a count-keyed staged-set watcher never re-fires and the earlier + * snapshot goes stale; (C) a review truncated by the file/byte cap only + * ever recorded the capped subset it actually reviewed, so comparing + * coverage against that same subset is a tautology. Recomputing here, + * fresh, right before the trailer is written, is correct regardless of + * watcher granularity or truncation. + */ +async function proceedToCommit(trailers: string) { + await commitReview.reconcileIterationsForHead(repoFolderPath.value ?? ""); + + const gate = resolveCommitReviewGate({ + enabled: settings.value.commitReviewEnabled, + staged: repoStats.value.staged, + decision: commitReviewDecision.value, + iterations: commitReview.iterations.value, + }); + if (gate === "prompt") { + pendingCommitReviewTrailers = trailers; + showCommitReviewDecisionModal.value = true; + return; + } + + const decision = effectiveReviewDecision(commitReviewDecision.value, commitReview.iterations.value); + const coverageNow = decision + ? await commitReview.computeCurrentCoverage(repoFolderPath.value ?? "") + : commitReview.coverage.value; + const reviewTrailerLine = decision + ? buildReviewTrailer(decision, commitReview.iterations.value, coverageNow) + : ""; + const fullTrailers = appendReviewTrailer(trailers, reviewTrailerLine); + + // v3.7.0 (Task 4) — `lastCommitHash` only changes on a SUCCESSFUL commit + // (`useGitRepo.commit()` catches its own errors internally, never + // throws) — comparing before/after is the cleanest local success signal, + // without adding a global watcher that would also have to special-case + // amend/other commit paths. A new commit starts a new review cycle. + const beforeHash = lastCommitHash.value; + await doCommit(fullTrailers); + if (lastCommitHash.value !== beforeHash) { + commitReview.clearReviewState(repoFolderPath.value ?? ""); + commitReviewDecision.value = null; + } +} + +/** + * "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; + const ran = await onReviewStagedClicked(); + if (!ran) { + repoError.value = t("errors.noAiProviderShort"); + return; + } + if (!commitReview.lastError.value) { + showCommitReviewModal.value = true; + } +} + +async function onCommitReviewDecisionVouch() { + commitReviewDecision.value = "vouched"; + showCommitReviewDecisionModal.value = false; + await proceedToCommit(pendingCommitReviewTrailers); +} + +async function onCommitReviewDecisionSkip() { + commitReviewDecision.value = "skipped"; + showCommitReviewDecisionModal.value = false; + await proceedToCommit(pendingCommitReviewTrailers); +} + +/** Decision D8 — cancelling (Escape/backdrop/Cancel button) cancels the + * commit outright. It must NEVER silently record "skipped": skipping is + * only ever the result of an explicit click on the Skip button. */ +function onCommitReviewDecisionCancel() { + showCommitReviewDecisionModal.value = false; + pendingCommitReviewTrailers = ""; } /** @@ -1146,7 +1289,7 @@ async function onSecretsIgnore(patternId: string) { function onSecretsCommitAnyway() { showSecretsModal.value = false; - void doCommit(lastAttemptedCommitTrailers); + void proceedToCommit(lastAttemptedCommitTrailers); } /** v3.7.0 — "Review staged changes" button handler. Shows a brief clean-pass @@ -1154,12 +1297,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 @@ -1207,22 +1355,31 @@ const repoSidebarListeners = { // opened/switched. Never a setInterval — see apps/desktop/CLAUDE.md P6.4. watch( () => [repoFolderPath.value, repoStats.value.staged] as const, - ([cwd]) => { + ([cwd, staged]) => { if (cwd) { secretsScanner.scan(cwd, settings.value); } 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(); + // findings are on screen (the diff they reviewed no longer exists — D5, + // no auto-run on a plain staging change). `onStagedSetChanged` is also + // the real trigger for Task 3's "re-review on the next staging change" + // after a "Fix with agent" handoff: it's a no-op unless `armReReview()` + // was called, in which case it fires exactly one re-review here. + commitReview.onStagedSetChanged(cwd ?? "", locale.value, staged); }, { immediate: true }, ); +// Task 5 — a repo switch starts a fresh commit-review decision cycle: forget +// whatever Vouch/Skip/ran decision applied to the PREVIOUS repo. Deliberately +// separate from the staged-set watcher above — the decision must survive a +// plain staging change within the SAME repo (e.g. the "Review now" round trip). +watch(repoFolderPath, () => { + commitReviewDecision.value = null; +}); + function onDiscardSection(sectionKey: string, paths: string[]) { discardSectionConfirm.value = { sectionKey, paths }; } @@ -1802,28 +1959,48 @@ function onNewAiTask() { aiTaskNamePrompt.value = true; } +/** + * v3.7.0 (Task 3) — the scratch-worktree creation sequence used by "New AI + * task" (`confirmNewAiTask`): create -> register -> select -> open. Returns + * `null` (without throwing) when there's no active project tab to base the + * scratch on — the caller decides how to surface that. + * + * Commit Review's "Fix with agent" used to also call this (optionally + * opening the agent in a scratch worktree), but that option was removed + * (see `onCommitReviewFixWithAgent`'s doc comment) after manual QA found a + * brand-new scratch worktree always hits a first-run onboarding screen that + * misinterprets the piped prompt. This function is no longer reachable from + * Commit Review at all — only `confirmNewAiTask` calls it now. + */ +async function createAiTaskScratchWorktree(name?: string): Promise { + if (!repoFolderPath.value || activeTabId.value === null) return null; + // Always base the scratch on the active project's root, not whatever + // worktree is currently selected, so AI tasks branch from the project. + const projectTab = repoTabs.value.find((t) => t.id === activeTabId.value); + const origin = projectTab?.path ?? repoFolderPath.value; + const scratch = await scratchWorktreeCreate(origin, undefined, name || undefined); + aiTasks.register({ + path: scratch.path, + originCwd: origin, + branch: scratch.branch, + createdAt: scratch.created_at, + }); + void refreshWorktreeCount(origin); + // Switch the project's checkout to the new scratch worktree in place, then + // load it so a spawned agent terminal lands in the right cwd. + selectWorktree(activeTabId.value, scratch.path); + await openRepo(scratch.path); + return scratch; +} + /** Create the AI-task scratch worktree once the user has named it. */ async function confirmNewAiTask(name: string) { if (!repoFolderPath.value || activeTabId.value === null) return; aiTaskNameBusy.value = true; try { - // Always base the scratch on the active project's root, not whatever - // worktree is currently selected, so AI tasks branch from the project. - const projectTab = repoTabs.value.find((t) => t.id === activeTabId.value); - const origin = projectTab?.path ?? repoFolderPath.value; - const scratch = await scratchWorktreeCreate(origin, undefined, name || undefined); - aiTasks.register({ - path: scratch.path, - originCwd: origin, - branch: scratch.branch, - createdAt: scratch.created_at, - }); - void refreshWorktreeCount(origin); + const scratch = await createAiTaskScratchWorktree(name); aiTaskNamePrompt.value = false; - // Switch the project's checkout to the new scratch worktree in place, then - // load it and spawn the agent terminal there. - selectWorktree(activeTabId.value, scratch.path); - await openRepo(scratch.path); + if (!scratch) return; await openTerminalTab(scratch.path, "claude"); } catch (err) { aiTaskNamePrompt.value = false; @@ -1833,6 +2010,63 @@ async function confirmNewAiTask(name: string) { } } +/** + * v3.7.0 (Task 3) — "Fix with agent" from `CommitReviewModal`: types the + * 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). + * + * 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`). + * + * 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 }) { + const prompt = buildReviewFixPrompt(commitReview.findings.value); + if (!prompt) return; + showCommitReviewModal.value = false; + try { + 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 + // explicitly anyway (per plan) rather than relying on that implicitly. + 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) { + reportAgentLaunchError(payload.tool, err); + } +} + /** * Definitively close a repo (project) tab. Closing a project never deletes its * worktrees — it only drops the tab — so we just confirm (the new submenu @@ -3842,11 +4076,26 @@ onUnmounted(() => { :findings="commitReview.findings.value" :summary="commitReview.summary.value" :truncated="commitReview.truncated.value" + :iterations="commitReview.iterations.value" + :coverage="commitReview.coverage.value" @jump="onJumpToCommitReviewFinding($event)" @dismiss="commitReview.dismiss($event)" + @fix-with-agent="onCommitReviewFixWithAgent($event)" @close="showCommitReviewModal = false" /> + + +