From b277dcdcdfd4fcf6ad902b768bc0c809d16a9cc2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Fri, 10 Jul 2026 16:04:12 +0100 Subject: [PATCH] fix: simplify walkthrough prompt requests (#1660) * fix: simplify walkthrough prompt requests * fix: address walkthrough review feedback --- apps/backend/cmd/mock-agent/handler.go | 8 +- .../backend/cmd/mock-agent/mock_agent_test.go | 6 +- apps/backend/cmd/mock-agent/scenarios.go | 4 +- .../config/prompts/changes-walkthrough.md | 10 ++- .../internal/prompts/store/sqlite_test.go | 6 +- apps/web/components/review/review-dialog.tsx | 21 ++++- apps/web/components/review/review-top-bar.tsx | 8 +- .../components/task/changes-panel-data.tsx | 23 +++-- apps/web/components/task/changes-panel.tsx | 4 +- .../task/mobile/mobile-changes-panel.tsx | 4 +- .../components/task/task-changes-panel.tsx | 9 +- .../tests/review/mobile-walkthrough.spec.ts | 2 + apps/web/e2e/tests/review/walkthrough.spec.ts | 29 ++++--- .../use-request-changes-walkthrough.test.ts | 17 ++-- .../use-request-changes-walkthrough.ts | 11 ++- apps/web/lib/walkthrough-prompt-files.test.ts | 56 ------------- apps/web/lib/walkthrough-prompt-files.ts | 78 ----------------- apps/web/lib/walkthrough-request.test.ts | 84 ++++++++----------- apps/web/lib/walkthrough-request.ts | 65 +++----------- 19 files changed, 142 insertions(+), 303 deletions(-) delete mode 100644 apps/web/lib/walkthrough-prompt-files.test.ts delete mode 100644 apps/web/lib/walkthrough-prompt-files.ts diff --git a/apps/backend/cmd/mock-agent/handler.go b/apps/backend/cmd/mock-agent/handler.go index 781df7b417..3df5e6d890 100644 --- a/apps/backend/cmd/mock-agent/handler.go +++ b/apps/backend/cmd/mock-agent/handler.go @@ -26,7 +26,10 @@ const overloaded529Message = "Internal error: API Error: 529 Overloaded. This is // before recovering (default 1, so the turn self-heals on the first retry). // Use a large N (e.g. `/overloaded:9`) to exhaust the retry budget and fall // through to the red recovery banner. -var overloadedCmdRe = regexp.MustCompile(`(?i)^/(?:e2e:)?overloaded(?::(\d+))?$`) +var ( + overloadedCmdRe = regexp.MustCompile(`(?i)^/(?:e2e:)?overloaded(?::(\d+))?$`) + changesWalkthroughPromptRefRe = regexp.MustCompile(`^@changes-walkthrough(?:\s|$)`) +) const changesWalkthroughPromptMarker = "Please create an agent-authored walkthrough of the current changes" @@ -35,8 +38,7 @@ func isChangesWalkthroughRequest(prompt string) bool { legacyPrompt := strings.Contains(cmd, changesWalkthroughPromptMarker) && strings.Contains(cmd, "show_walkthrough_kandev") && strings.Contains(cmd, "Available changed files:") - promptReference := strings.Contains(cmd, "@changes-walkthrough") && - strings.Contains(cmd, "Changed files:") + promptReference := changesWalkthroughPromptRefRe.MatchString(cmd) return legacyPrompt || promptReference } diff --git a/apps/backend/cmd/mock-agent/mock_agent_test.go b/apps/backend/cmd/mock-agent/mock_agent_test.go index e6c3a92713..054e5692e8 100644 --- a/apps/backend/cmd/mock-agent/mock_agent_test.go +++ b/apps/backend/cmd/mock-agent/mock_agent_test.go @@ -124,9 +124,6 @@ func TestIsChangesWalkthroughRequest(t *testing.T) { promptReference := strings.Join([]string{ "@changes-walkthrough", "", - "Changed files:", - "- src/app.ts [uncommitted]", - "", "", "EXPANDED PROMPT REFERENCES", "### @changes-walkthrough", @@ -142,6 +139,9 @@ func TestIsChangesWalkthroughRequest(t *testing.T) { if isChangesWalkthroughRequest("show_walkthrough_kandev without the generated prompt shape") { t.Fatal("expected unrelated prompt not to be detected") } + if isChangesWalkthroughRequest("what does @changes-walkthrough do?") { + t.Fatal("expected incidental prompt reference not to be detected") + } } func TestDelayRange(t *testing.T) { diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 0ff7de86a1..86801a5828 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -851,8 +851,8 @@ func walkthroughDemoArgs() map[string]interface{} { return map[string]interface{}{ wtKeyTitle: "Tour of the change", wtKeySteps: []map[string]interface{}{ - wtStep("Entry point", "walkthrough_a.txt", - "Execution starts here. **Step 1** explains the entry point in file A.", 1, 0), + wtStep("Overview", "walkthrough_a.txt", + "Big picture: this tour explains how the sample changes connect across the touched files.\n\nELI5: first we show the map, then we explain each changed line.", 1, 0), wtStep("The change in A", "walkthrough_a.txt", "Step 2: this is WALKTHROUGH_CHANGE_A the tour narrates.", 2, 3), wtStep("File B", "walkthrough_b.txt", diff --git a/apps/backend/config/prompts/changes-walkthrough.md b/apps/backend/config/prompts/changes-walkthrough.md index cba51bb9e9..c66cfd23aa 100644 --- a/apps/backend/config/prompts/changes-walkthrough.md +++ b/apps/backend/config/prompts/changes-walkthrough.md @@ -1,12 +1,14 @@ Please create an agent-authored walkthrough of the current changes using `show_walkthrough_kandev`. Walkthrough requirements: -- Use only files listed below or files you verify exist in this task's local worktree/review diff. +- Inspect the changed files yourself instead of relying on UI-provided paths or diff context. +- If this is a PR task or PR review task, compare the PR head against the PR base branch. +- Otherwise, compare against the task/repository base using the local task state (`git status`, `git diff`, `git diff --cached`, and cumulative diff context when available). - For PR-only files, do not assume the PR head is checked out locally; anchor to the review diff when available, and avoid editor-only/current-worktree claims. +- The first walkthrough step must contextualize the whole change before explaining individual hunks. +- Anchor the first walkthrough step to the most representative changed line or changed line range available. +- Include an `ELI5:` line in the first walkthrough step text that explains the change in the simplest terms possible. - Anchor steps to changed lines or changed line ranges whenever possible. - Use `line_end` whenever a logical explanation spans multiple lines; prefer one range step over adjacent single-line steps. - Keep each step concise and direct. Do not include a `Justification:` preamble. - If a good local/review anchor is unavailable, omit that step instead of referencing a remote-only path. - -Available changed files: -{{changed_files}} diff --git a/apps/backend/internal/prompts/store/sqlite_test.go b/apps/backend/internal/prompts/store/sqlite_test.go index 89e54275ec..25cb47db34 100644 --- a/apps/backend/internal/prompts/store/sqlite_test.go +++ b/apps/backend/internal/prompts/store/sqlite_test.go @@ -164,9 +164,13 @@ func TestSQLiteRepository_BuiltinPrompts(t *testing.T) { } for _, want := range []string{ "show_walkthrough_kandev", - "{{changed_files}}", + "Inspect the changed files yourself", + "compare the PR head against the PR base branch", + "compare against the task/repository base", "Use `line_end`", "do not assume the PR head is checked out locally", + "first walkthrough step", + "ELI5:", } { if !strings.Contains(changesWalkthroughContent, want) { t.Fatalf("expected changes-walkthrough prompt to contain %q, got:\n%s", want, changesWalkthroughContent) diff --git a/apps/web/components/review/review-dialog.tsx b/apps/web/components/review/review-dialog.tsx index 4f87ed0f17..e5ca74dc59 100644 --- a/apps/web/components/review/review-dialog.tsx +++ b/apps/web/components/review/review-dialog.tsx @@ -468,14 +468,27 @@ function useWalkthroughFileSelection( }, [open, step, allFiles, filter, setFilter, handleSelectFile]); } +function useReviewDialogWalkthroughRequest({ + sessionId, + allFiles, +}: { + sessionId: string; + allFiles: ReviewFile[]; +}) { + const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); + return useRequestChangesWalkthrough({ + taskId: activeTaskId, + sessionId, + ready: allFiles.length > 0, + }); +} + export const ReviewDialog = memo(function ReviewDialog(props: ReviewDialogProps) { const { open, onOpenChange, sessionId, baseBranch, onOpenFile } = props; const s = useReviewDialogState(props); - const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); - const handleRequestWalkthrough = useRequestChangesWalkthrough({ - taskId: activeTaskId, + const handleRequestWalkthrough = useReviewDialogWalkthroughRequest({ sessionId, - files: s.allFiles, + allFiles: s.allFiles, }); const splitRowRef = useRef(null); const sidebar = useReviewSidebarResize(splitRowRef, open); diff --git a/apps/web/components/review/review-top-bar.tsx b/apps/web/components/review/review-top-bar.tsx index 9159e01bf4..db55142164 100644 --- a/apps/web/components/review/review-top-bar.tsx +++ b/apps/web/components/review/review-top-bar.tsx @@ -225,7 +225,13 @@ export const ReviewTopBar = memo(function ReviewTopBar({ disabled={requestWalkthroughDisabled} /> - diff --git a/apps/web/components/task/changes-panel-data.tsx b/apps/web/components/task/changes-panel-data.tsx index f294eb05a4..4de791a7ee 100644 --- a/apps/web/components/task/changes-panel-data.tsx +++ b/apps/web/components/task/changes-panel-data.tsx @@ -27,8 +27,6 @@ import { buildRepoNameById, } from "./changes-panel-helpers"; import type { OpenDiffOptions } from "./changes-diff-target"; -import type { WalkthroughPromptFile } from "@/lib/walkthrough-request"; -import { buildWalkthroughPromptFiles } from "@/lib/walkthrough-prompt-files"; function useChangesPanelStoreData() { const activeTaskId = useAppStore((state) => state.tasks.activeTaskId); @@ -211,6 +209,10 @@ function useChangesPanelPRData() { return { prDiffFiles, prCommitsList, hasPRFiles, hasPRCommits, prFiles }; } +function hasCumulativeFiles(files: Record | null | undefined): boolean { + return Object.keys(files ?? {}).length > 0; +} + export function useChangesPanelData() { const { activeTaskId, activeSessionId, baseBranch, existingPrUrl } = useChangesPanelStoreData(); const baseBranchByRepo = useBaseBranchByRepo(activeTaskId); @@ -244,16 +246,11 @@ export function useChangesPanelData() { () => buildPrByRepoMap(taskPRsForMap, repoNameById, pendingByRepo), [taskPRsForMap, repoNameById, pendingByRepo], ); - const walkthroughPromptFiles = useMemo( - () => - buildWalkthroughPromptFiles({ - unstagedFiles, - stagedFiles, - committedFiles: git.statusLoaded ? git.cumulativeDiff?.files : null, - prFiles: prData.prFiles, - }), - [git.cumulativeDiff?.files, git.statusLoaded, prData.prFiles, stagedFiles, unstagedFiles], - ); + const walkthroughRequestReady = + unstagedFiles.length > 0 || + stagedFiles.length > 0 || + (git.statusLoaded && hasCumulativeFiles(git.cumulativeDiff?.files)) || + prData.prFiles.length > 0; return { activeTaskId, activeSessionId, @@ -272,7 +269,7 @@ export function useChangesPanelData() { repoDisplayName, prByRepo, existingPrUrl, - walkthroughPromptFiles, + walkthroughRequestReady, ...prData, }; } diff --git a/apps/web/components/task/changes-panel.tsx b/apps/web/components/task/changes-panel.tsx index 2bee1c999d..c4ad2889c3 100644 --- a/apps/web/components/task/changes-panel.tsx +++ b/apps/web/components/task/changes-panel.tsx @@ -26,7 +26,7 @@ const ChangesPanel = memo(function ChangesPanel(props: ChangesPanelProps) { const requestWalkthrough = useRequestChangesWalkthrough({ taskId: data.activeTaskId, sessionId: data.activeSessionId, - files: data.walkthroughPromptFiles, + ready: data.walkthroughRequestReady, }); if (isArchived) return ; return ( @@ -44,7 +44,7 @@ const ChangesPanel = memo(function ChangesPanel(props: ChangesPanelProps) { onOpenDiffAll={props.onOpenDiffAll} onOpenReview={props.onOpenReview} onRequestWalkthrough={requestWalkthrough} - requestWalkthroughDisabled={data.walkthroughPromptFiles.length === 0} + requestWalkthroughDisabled={!data.walkthroughRequestReady} repoNames={data.git.repoNames} perRepoStatus={data.git.perRepoStatus} onRepoPull={data.repoCallbacks.onRepoPull} diff --git a/apps/web/components/task/mobile/mobile-changes-panel.tsx b/apps/web/components/task/mobile/mobile-changes-panel.tsx index 955a79e1ac..95c12e2960 100644 --- a/apps/web/components/task/mobile/mobile-changes-panel.tsx +++ b/apps/web/components/task/mobile/mobile-changes-panel.tsx @@ -38,7 +38,7 @@ export const MobileChangesPanel = memo(function MobileChangesPanel({ const requestWalkthrough = useRequestChangesWalkthrough({ taskId: data.activeTaskId, sessionId: data.activeSessionId, - files: data.walkthroughPromptFiles, + ready: data.walkthroughRequestReady, }); // Track the previous selectedDiff to detect changes @@ -107,7 +107,7 @@ export const MobileChangesPanel = memo(function MobileChangesPanel({ onOpenDiffAll={handleOpenDiffAll} onOpenReview={handleOpenReview} onRequestWalkthrough={requestWalkthrough} - requestWalkthroughDisabled={data.walkthroughPromptFiles.length === 0} + requestWalkthroughDisabled={!data.walkthroughRequestReady} repoNames={data.git.repoNames} perRepoStatus={data.git.perRepoStatus} onRepoPull={data.repoCallbacks.onRepoPull} diff --git a/apps/web/components/task/task-changes-panel.tsx b/apps/web/components/task/task-changes-panel.tsx index 99d0eb3ea2..c9e7b9283b 100644 --- a/apps/web/components/task/task-changes-panel.tsx +++ b/apps/web/components/task/task-changes-panel.tsx @@ -169,15 +169,12 @@ function persistAutoMarkSetting(checked: boolean) { updateUserSettings(payload, { cache: "no-store" }).catch(() => {}); } -function useReviewWalkthroughRequest( - activeSessionId: string | null | undefined, - allFiles: ReviewFile[], -) { +function useWalkthroughRequest(activeSessionId: string | null | undefined, allFiles: ReviewFile[]) { const activeTaskId = useAppStore((s) => s.tasks.activeTaskId); return useRequestChangesWalkthrough({ taskId: activeTaskId, sessionId: activeSessionId, - files: allFiles, + ready: allFiles.length > 0, }); } @@ -485,7 +482,7 @@ const TaskChangesPanel = memo(function TaskChangesPanel({ handleToggleAutoMark, handleFixComments, } = useChangesActions(activeSessionId, allFiles, wordWrapProp); - const handleRequestWalkthrough = useReviewWalkthroughRequest(activeSessionId, allFiles); + const handleRequestWalkthrough = useWalkthroughRequest(activeSessionId, allFiles); const { visibleFiles, visibleFileRefs, reviewedCount, totalCount, progressPercent } = useVisibleDiffState({ allFiles, diff --git a/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts b/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts index 5dd1b094b2..9302e37c8c 100644 --- a/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts +++ b/apps/web/e2e/tests/review/mobile-walkthrough.spec.ts @@ -88,6 +88,8 @@ test.describe("Mobile code walkthrough", () => { const card = session.walkthroughFloating(); await expect(card).toBeVisible({ timeout: 30_000 }); await expect(card).toHaveAttribute("data-mobile-variant", "bottom-sheet"); + await expect(card.getByTestId("walkthrough-step-title")).toContainText("Overview"); + await expect(card.getByTestId("walkthrough-step-body")).toContainText("ELI5:"); await expect(session.walkthroughEditorRange()).toBeVisible({ timeout: 15_000 }); }); diff --git a/apps/web/e2e/tests/review/walkthrough.spec.ts b/apps/web/e2e/tests/review/walkthrough.spec.ts index 8268929fe3..cd9819526b 100644 --- a/apps/web/e2e/tests/review/walkthrough.spec.ts +++ b/apps/web/e2e/tests/review/walkthrough.spec.ts @@ -52,12 +52,15 @@ async function customizeChangesWalkthroughPrompt( "Please create an agent-authored walkthrough of the current changes using `show_walkthrough_kandev`.", "", "Walkthrough requirements:", + "- Inspect the changed files yourself instead of relying on UI-provided paths.", + "- For PR tasks, compare the PR head against the PR base branch.", + "- For non-PR tasks, compare against the task or repository base.", + "- The first walkthrough step must contextualize the whole change.", + "- Anchor the first step to the most representative changed line or range.", + "- Include an `ELI5:` line in the first step text.", "- Use `line_end` whenever a logical explanation spans multiple lines.", "- For PR-only files, do not assume the PR head is checked out locally.", "- Keep each step concise and direct. Do not include a `Justification:` preamble.", - "", - "Available changed files:", - "{{changed_files}}", ].join("\n"), }); return () => apiClient.updatePrompt(prompt!.id, { content: originalContent }); @@ -97,8 +100,10 @@ test.describe("Code walkthrough", () => { await expect(session.activeChat()).toContainText("@changes-walkthrough", { timeout: 15_000, }); - await expect(session.activeChat()).toContainText("Changed files:"); - await expect(session.activeChat()).toContainText("walkthrough_a.txt ["); + await expect(session.activeChat()).not.toContainText("Diff context:"); + await expect(session.activeChat()).not.toContainText("Base branch:"); + await expect(session.activeChat()).not.toContainText("Changed files:"); + await expect(session.activeChat()).not.toContainText("walkthrough_a.txt ["); await expect(session.activeChat()).not.toContainText("E2E_CUSTOM_CHANGES_WALKTHROUGH"); await expect(session.activeChat()).toContainText("Walkthrough: Tour of the change", { timeout: 45_000, @@ -110,7 +115,8 @@ test.describe("Code walkthrough", () => { const card = await openWalkthrough(testPage); await expect(card.getByTestId("walkthrough-step-header")).toContainText("Step 1 / 5"); - await expect(card.getByTestId("walkthrough-step-body")).toContainText("Step 1"); + await expect(card.getByTestId("walkthrough-step-title")).toContainText("Overview"); + await expect(card.getByTestId("walkthrough-step-body")).toContainText("ELI5:"); await expect(session.walkthroughEditorRange()).toBeVisible({ timeout: 15_000 }); } finally { await restorePrompt(); @@ -137,15 +143,17 @@ test.describe("Code walkthrough", () => { await expect(request).toHaveAttribute("aria-label", "Walk me through these review changes"); await request.click(); - await testPage.keyboard.press("Escape"); + await dialog.getByRole("button", { name: "Close review" }).click(); await expect(dialog).toBeHidden({ timeout: 15_000 }); await session.showSessionContext(); await expect(session.activeChat()).toContainText("@changes-walkthrough", { timeout: 15_000, }); - await expect(session.activeChat()).toContainText("Changed files:"); - await expect(session.activeChat()).toContainText("walkthrough_a.txt ["); + await expect(session.activeChat()).not.toContainText("Diff context:"); + await expect(session.activeChat()).not.toContainText("Base branch:"); + await expect(session.activeChat()).not.toContainText("Changed files:"); + await expect(session.activeChat()).not.toContainText("walkthrough_a.txt ["); await expect(session.activeChat()).not.toContainText("E2E_CUSTOM_CHANGES_WALKTHROUGH"); await expect(session.activeChat()).toContainText("walkthrough-request complete", { timeout: 45_000, @@ -166,7 +174,8 @@ test.describe("Code walkthrough", () => { const body = card.getByTestId("walkthrough-step-body"); await expect(header).toContainText("Step 1 / 5"); - await expect(body).toContainText("Step 1"); + await expect(card.getByTestId("walkthrough-step-title")).toContainText("Overview"); + await expect(body).toContainText("ELI5:"); await expect(card.getByTestId("walkthrough-step-file")).toContainText("walkthrough_a.txt"); await expect(card.getByTestId("walkthrough-prev")).toBeDisabled(); diff --git a/apps/web/hooks/domains/session/use-request-changes-walkthrough.test.ts b/apps/web/hooks/domains/session/use-request-changes-walkthrough.test.ts index 36f1ce3695..02f519bf55 100644 --- a/apps/web/hooks/domains/session/use-request-changes-walkthrough.test.ts +++ b/apps/web/hooks/domains/session/use-request-changes-walkthrough.test.ts @@ -40,12 +40,12 @@ function storeState(sessionState: string, planMode = false) { }; } -function renderRequestHook(files = [{ path: "src/app.ts", source: "uncommitted" }]) { +function renderRequestHook(ready = true) { return renderHook(() => useRequestChangesWalkthrough({ taskId: "task-1", sessionId: "session-1", - files, + ready, }), ); } @@ -59,7 +59,7 @@ describe("useRequestChangesWalkthrough", () => { { id: "builtin-changes-walkthrough", name: "changes-walkthrough", - content: "CUSTOM_WALKTHROUGH_PROMPT\n{{changed_files}}\nshow_walkthrough_kandev", + content: "CUSTOM_WALKTHROUGH_PROMPT\nshow_walkthrough_kandev", builtin: true, }, ], @@ -87,13 +87,16 @@ describe("useRequestChangesWalkthrough", () => { expect.objectContaining({ task_id: "task-1", session_id: "session-1", - content: expect.stringMatching(/^@changes-walkthrough\n\nChanged files:/), + content: expect.stringMatching(/^@changes-walkthrough\n\n/), }), 10000, ); const sentContent = mockRequest.mock.calls[0]?.[1]?.content as string; expect(sentContent).toContain(""); expect(sentContent).toContain("CUSTOM_WALKTHROUGH_PROMPT"); + expect(sentContent).not.toContain("Diff context:"); + expect(sentContent).not.toContain("Base branch:"); + expect(sentContent).not.toContain("Base commit:"); expect(mockListPrompts).toHaveBeenCalledWith({ cache: "no-store" }); expect(mockAddMessage).toHaveBeenCalledWith(expect.objectContaining({ id: "msg-1" })); expect(mockQueueMessage).not.toHaveBeenCalled(); @@ -115,7 +118,7 @@ describe("useRequestChangesWalkthrough", () => { session_id: "session-1", task_id: "task-1", plan_mode: true, - content: expect.stringMatching(/^@changes-walkthrough\n\nChanged files:/), + content: expect.stringMatching(/^@changes-walkthrough\n\n/), }), ); const queuedContent = mockQueueMessage.mock.calls[0]?.[0]?.content as string; @@ -127,8 +130,8 @@ describe("useRequestChangesWalkthrough", () => { ); }); - it("does not send a low-context prompt before changed files are loaded", async () => { - const { result } = renderRequestHook([]); + it("does not send a low-context prompt before diff context is ready", async () => { + const { result } = renderRequestHook(false); await act(async () => { await result.current(); diff --git a/apps/web/hooks/domains/session/use-request-changes-walkthrough.ts b/apps/web/hooks/domains/session/use-request-changes-walkthrough.ts index ac0a003851..95daa4de81 100644 --- a/apps/web/hooks/domains/session/use-request-changes-walkthrough.ts +++ b/apps/web/hooks/domains/session/use-request-changes-walkthrough.ts @@ -7,7 +7,6 @@ import { getWebSocketClient } from "@/lib/ws/connection"; import { buildChangesWalkthroughPrompt, CHANGES_WALKTHROUGH_PROMPT_NAME, - type WalkthroughPromptFile, } from "@/lib/walkthrough-request"; import type { Message } from "@/lib/types/http"; import type { AppState } from "@/lib/state/store"; @@ -15,7 +14,7 @@ import type { AppState } from "@/lib/state/store"; type UseRequestChangesWalkthroughParams = { taskId: string | null | undefined; sessionId: string | null | undefined; - files: WalkthroughPromptFile[]; + ready?: boolean; }; function isAgentBusy(state: string | undefined): boolean { @@ -75,7 +74,7 @@ async function sendWalkthroughRequest(params: { export function useRequestChangesWalkthrough({ taskId, sessionId, - files, + ready = true, }: UseRequestChangesWalkthroughParams) { const storeApi = useAppStoreApi(); const { toast } = useToast(); @@ -87,13 +86,13 @@ export function useRequestChangesWalkthrough({ const activeSession = state.taskSessions.items[sessionId] ?? null; const shouldQueue = isAgentBusy(activeSession?.state); const planModeEnabled = state.chatInput.planModeBySessionId[sessionId] ?? false; - if (files.length === 0) { + if (!ready) { toast({ title: "Changes are still loading", variant: "error" }); return; } try { const template = await loadChangesWalkthroughPromptTemplate(); - const content = buildChangesWalkthroughPrompt(template, files); + const content = buildChangesWalkthroughPrompt(template); if (shouldQueue) { await queueWalkthroughRequest({ taskId, @@ -111,5 +110,5 @@ export function useRequestChangesWalkthrough({ console.error("Failed to request walkthrough:", error); toast({ title: "Failed to request walkthrough", variant: "error" }); } - }, [files, sessionId, storeApi, taskId, toast]); + }, [ready, sessionId, storeApi, taskId, toast]); } diff --git a/apps/web/lib/walkthrough-prompt-files.test.ts b/apps/web/lib/walkthrough-prompt-files.test.ts deleted file mode 100644 index e94a96dc54..0000000000 --- a/apps/web/lib/walkthrough-prompt-files.test.ts +++ /dev/null @@ -1,56 +0,0 @@ -import { describe, expect, it } from "vitest"; -import { buildWalkthroughPromptFiles } from "./walkthrough-prompt-files"; - -const APP_PATH = "src/app.ts"; - -describe("buildWalkthroughPromptFiles", () => { - it("includes working tree, staged, committed, and PR files in review order", () => { - const files = buildWalkthroughPromptFiles({ - unstagedFiles: [{ path: APP_PATH }], - stagedFiles: [{ path: "src/staged.ts", repositoryName: "web" }], - committedFiles: { - "src/committed.ts": { path: "src/committed.ts" }, - }, - prFiles: [{ path: "src/pr.ts", repository_name: "backend" }], - }); - - expect(files).toEqual([ - { path: APP_PATH, source: "uncommitted" }, - { path: "src/staged.ts", repository_name: "web", source: "staged" }, - { path: "src/committed.ts", source: "committed" }, - { path: "src/pr.ts", repository_name: "backend", source: "pr" }, - ]); - }); - - it("keeps the freshest local source when cumulative diff repeats a file", () => { - const files = buildWalkthroughPromptFiles({ - unstagedFiles: [{ path: APP_PATH }], - stagedFiles: [], - committedFiles: { - [APP_PATH]: { path: APP_PATH }, - "src/other.ts": { path: "src/other.ts" }, - }, - prFiles: [], - }); - - expect(files).toEqual([ - { path: APP_PATH, source: "uncommitted" }, - { path: "src/other.ts", source: "committed" }, - ]); - }); - - it("falls back to composite cumulative-diff keys when the payload lacks path fields", () => { - const files = buildWalkthroughPromptFiles({ - unstagedFiles: [], - stagedFiles: [], - committedFiles: { - "backend\u0000src/server.go": {}, - }, - prFiles: [], - }); - - expect(files).toEqual([ - { path: "src/server.go", repository_name: "backend", source: "committed" }, - ]); - }); -}); diff --git a/apps/web/lib/walkthrough-prompt-files.ts b/apps/web/lib/walkthrough-prompt-files.ts deleted file mode 100644 index eb2dcb6da3..0000000000 --- a/apps/web/lib/walkthrough-prompt-files.ts +++ /dev/null @@ -1,78 +0,0 @@ -import type { WalkthroughPromptFile } from "./walkthrough-request"; - -type PromptFileInput = { - path?: string; - repositoryName?: string; - repository_name?: string; -}; - -type CumulativePromptFileInput = PromptFileInput & { - diff?: string; -}; - -type PromptFileSource = "uncommitted" | "staged" | "committed" | "pr"; - -const FILE_KEY_SEP = "\u0000"; - -function splitCompositeKey(key: string): { repo: string; path: string } { - const sep = key.indexOf(FILE_KEY_SEP); - if (sep < 0) return { repo: "", path: key }; - return { repo: key.slice(0, sep), path: key.slice(sep + 1) }; -} - -function normalizePromptFile( - source: PromptFileSource, - file: PromptFileInput, - fallbackKey?: string, -): WalkthroughPromptFile | null { - const fallback = fallbackKey ? splitCompositeKey(fallbackKey) : { repo: "", path: "" }; - const path = file.path || fallback.path; - if (!path) return null; - const repo = file.repository_name ?? file.repositoryName ?? fallback.repo; - return { - path, - ...(repo ? { repository_name: repo } : {}), - source, - }; -} - -function promptKey(file: WalkthroughPromptFile): string { - return `${file.repository_name ?? file.repositoryName ?? ""}\0${file.path}`; -} - -function addPromptFile( - out: WalkthroughPromptFile[], - seen: Set, - source: PromptFileSource, - file: PromptFileInput, - fallbackKey?: string, -) { - const promptFile = normalizePromptFile(source, file, fallbackKey); - if (!promptFile) return; - const key = promptKey(promptFile); - if (seen.has(key)) return; - seen.add(key); - out.push(promptFile); -} - -export function buildWalkthroughPromptFiles({ - unstagedFiles, - stagedFiles, - committedFiles, - prFiles, -}: { - unstagedFiles: PromptFileInput[]; - stagedFiles: PromptFileInput[]; - committedFiles?: Record | null; - prFiles: PromptFileInput[]; -}): WalkthroughPromptFile[] { - const out: WalkthroughPromptFile[] = []; - const seen = new Set(); - for (const file of unstagedFiles) addPromptFile(out, seen, "uncommitted", file); - for (const file of stagedFiles) addPromptFile(out, seen, "staged", file); - for (const [key, file] of Object.entries(committedFiles ?? {})) { - addPromptFile(out, seen, "committed", file, key); - } - for (const file of prFiles) addPromptFile(out, seen, "pr", file); - return out; -} diff --git a/apps/web/lib/walkthrough-request.test.ts b/apps/web/lib/walkthrough-request.test.ts index 28952f8c31..9588911d2f 100644 --- a/apps/web/lib/walkthrough-request.test.ts +++ b/apps/web/lib/walkthrough-request.test.ts @@ -1,66 +1,48 @@ import { describe, expect, it } from "vitest"; -import { - buildChangesWalkthroughPrompt, - formatChangedFilesForWalkthroughPrompt, -} from "./walkthrough-request"; +import { buildChangesWalkthroughPrompt } from "./walkthrough-request"; describe("buildChangesWalkthroughPrompt", () => { - it("sends the saved prompt reference visibly and hides the expanded prompt content", () => { - const prompt = buildChangesWalkthroughPrompt("CUSTOM_PROMPT\n{{changed_files}}", [ - { path: "src/app.ts", source: "uncommitted" }, - { path: "src/review.ts", repository_name: "web", source: "pr" }, - ]); + it("sends only the prompt reference visibly and hides expanded prompt content", () => { + const prompt = buildChangesWalkthroughPrompt("CUSTOM_PROMPT\nshow_walkthrough_kandev"); - expect(prompt).toMatch(/^@changes-walkthrough\n\nChanged files:/); + expect(prompt).toMatch(/^@changes-walkthrough\n\n/); expect(prompt).toContain(""); expect(prompt).toContain("EXPANDED PROMPT REFERENCES"); expect(prompt).toContain("### @changes-walkthrough"); expect(prompt).toContain("CUSTOM_PROMPT"); - expect(prompt).toContain("- src/app.ts [uncommitted]"); - expect(prompt).toContain("- web:src/review.ts [pr]"); + expect(prompt).not.toContain("Diff context:"); + expect(prompt).not.toContain("Base branch:"); + expect(prompt).not.toContain("Base commit:"); + expect(prompt).not.toContain("src/app.ts"); + }); + + it("maps diff context placeholders to static instructions instead of dynamic context", () => { + const prompt = buildChangesWalkthroughPrompt("CUSTOM\n{{diff_context}}"); + + expect(prompt).toContain("CUSTOM"); + expect(prompt).toContain("Inspect the task changes and compare against the correct base."); + expect(prompt).not.toContain("PR: kdlbs/kandev#42"); + expect(prompt).not.toContain("Head branch:"); + expect(prompt).not.toContain("Base branch:"); + expect(prompt).not.toContain("Changed files:"); + expect(prompt).not.toContain("{{diff_context}}"); + }); + + it("maps legacy changed_files placeholders to static instructions instead of file paths", () => { + const prompt = buildChangesWalkthroughPrompt("CUSTOM\n{{changed_files}}"); + + expect(prompt).toContain("CUSTOM"); + expect(prompt).toContain("Inspect the task changes and compare against the correct base."); + expect(prompt).not.toContain("Changed files:"); expect(prompt).not.toContain("{{changed_files}}"); }); - it("deduplicates repeated files and caps the file list", () => { - const many = Array.from({ length: 85 }, (_, index) => ({ - path: `src/file-${index}.ts`, - source: "committed", - })); - const prompt = buildChangesWalkthroughPrompt("{{changed_files}}", [many[0], ...many]); + it("does not append extra fallback instructions when a customized prompt omits placeholders", () => { + const prompt = buildChangesWalkthroughPrompt("CUSTOM_WITHOUT_PLACEHOLDER"); - expect(prompt.match(/src\/file-0\.ts/g)).toHaveLength(1); - expect(prompt).toContain("5 more file(s) omitted"); - }); - - it("escapes file metadata line breaks before prompt interpolation", () => { - const files = [ - { - path: "src/app.ts\n- injected", - repository_name: "web\nrepo", - source: "pr\rsource", - }, - ]; - - expect(formatChangedFilesForWalkthroughPrompt(files)).toContain( - "- web\\nrepo:src/app.ts\\n- injected [pr\\rsource]", - ); - }); - - it("keeps the changed file list visible when a customized prompt omits the placeholder", () => { - const prompt = buildChangesWalkthroughPrompt("CUSTOM_WITHOUT_PLACEHOLDER", [ - { path: "src/app.ts", source: "committed" }, - ]); - - expect(prompt).toMatch(/^@changes-walkthrough\n\nChanged files:/); + expect(prompt).toMatch(/^@changes-walkthrough\n\n/); expect(prompt).toContain("CUSTOM_WITHOUT_PLACEHOLDER"); - expect(prompt).toContain("- src/app.ts [committed]"); - }); -}); - -describe("formatChangedFilesForWalkthroughPrompt", () => { - it("renders an explicit loading fallback when no files are available", () => { - expect(formatChangedFilesForWalkthroughPrompt([])).toContain( - "No changed files were listed by the UI", - ); + expect(prompt).not.toContain("Diff context:"); + expect(prompt).not.toContain("No precomputed changed-file list is supplied."); }); }); diff --git a/apps/web/lib/walkthrough-request.ts b/apps/web/lib/walkthrough-request.ts index ea763a5262..09603f2d06 100644 --- a/apps/web/lib/walkthrough-request.ts +++ b/apps/web/lib/walkthrough-request.ts @@ -1,69 +1,26 @@ import { formatPromptReferenceExpansions } from "@/lib/prompts/expand-prompt-references"; -export type WalkthroughPromptFile = { - path: string; - repository_name?: string; - repositoryName?: string; - source?: "uncommitted" | "committed" | "pr" | string; -}; - -const MAX_PROMPT_FILES = 80; export const CHANGES_WALKTHROUGH_PROMPT_NAME = "changes-walkthrough"; + +// Custom prompts may still contain placeholders from earlier walkthrough prompt iterations. +const DIFF_CONTEXT_PLACEHOLDER = "{{diff_context}}"; const CHANGED_FILES_PLACEHOLDER = "{{changed_files}}"; -const CHANGED_FILES_REFERENCE = "The changed files are listed in the user-visible message above."; +const STATIC_CONTEXT_INSTRUCTION = "Inspect the task changes and compare against the correct base."; -function fileKey(file: WalkthroughPromptFile): string { - return `${file.repository_name ?? file.repositoryName ?? ""}\0${file.path}\0${file.source ?? ""}`; +function buildExpansionContent(template: string): string { + return template + .trim() + .replaceAll(DIFF_CONTEXT_PLACEHOLDER, STATIC_CONTEXT_INSTRUCTION) + .replaceAll(CHANGED_FILES_PLACEHOLDER, STATIC_CONTEXT_INSTRUCTION); } -function escapePromptFilePart(value: string): string { - return value.replaceAll("\\", "\\\\").replaceAll("\r", "\\r").replaceAll("\n", "\\n"); -} - -function formatPromptFile(file: WalkthroughPromptFile): string { - const repo = escapePromptFilePart(file.repository_name ?? file.repositoryName ?? ""); - const path = escapePromptFilePart(file.path); - const source = file.source ? ` [${escapePromptFilePart(file.source)}]` : ""; - return repo ? `${repo}:${path}${source}` : `${path}${source}`; -} - -export function formatChangedFilesForWalkthroughPrompt(files: WalkthroughPromptFile[]): string { - const uniqueFiles: WalkthroughPromptFile[] = []; - const seen = new Set(); - for (const file of files) { - if (!file.path) continue; - const key = fileKey(file); - if (seen.has(key)) continue; - seen.add(key); - uniqueFiles.push(file); - } - - const shown = uniqueFiles.slice(0, MAX_PROMPT_FILES); - const omitted = uniqueFiles.length - shown.length; - return shown.length > 0 - ? shown.map((file) => `- ${formatPromptFile(file)}`).join("\n") + - (omitted > 0 ? `\n- ... ${omitted} more file(s) omitted from this prompt` : "") - : "- No changed files were listed by the UI; inspect the local task state before anchoring."; -} - -export function buildChangesWalkthroughPrompt( - template: string, - files: WalkthroughPromptFile[], -): string { - const changedFiles = formatChangedFilesForWalkthroughPrompt(files); - const trimmedTemplate = template.trim(); - const expansionContent = trimmedTemplate.includes(CHANGED_FILES_PLACEHOLDER) - ? trimmedTemplate.replaceAll(CHANGED_FILES_PLACEHOLDER, CHANGED_FILES_REFERENCE) - : trimmedTemplate; +export function buildChangesWalkthroughPrompt(template: string): string { const expansionContext = formatPromptReferenceExpansions([ - { name: CHANGES_WALKTHROUGH_PROMPT_NAME, content: expansionContent }, + { name: CHANGES_WALKTHROUGH_PROMPT_NAME, content: buildExpansionContent(template) }, ]); return [ `@${CHANGES_WALKTHROUGH_PROMPT_NAME}`, "", - "Changed files:", - changedFiles, - "", "", expansionContext, "",