From 815fb5b24e30591c89eb0b7fc2e01017be851684 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Thu, 5 Mar 2026 15:18:30 +0000 Subject: [PATCH] fix: refresh diff view when untracked files change (#372) --- apps/backend/cmd/mock-agent/scenarios.go | 44 ++++++- .../server/process/workspace_files.go | 19 +-- .../server/process/workspace_monitor.go | 79 ++++++++++- apps/web/e2e/tests/diff-update.spec.ts | 123 +++++++++++++++--- 4 files changed, 235 insertions(+), 30 deletions(-) diff --git a/apps/backend/cmd/mock-agent/scenarios.go b/apps/backend/cmd/mock-agent/scenarios.go index 1ede73bc1d..f23e7b9581 100644 --- a/apps/backend/cmd/mock-agent/scenarios.go +++ b/apps/backend/cmd/mock-agent/scenarios.go @@ -36,12 +36,16 @@ func emitPredefinedScenario(enc *json.Encoder, scanner *bufio.Scanner, name stri scenarioDiffUpdateSetup(enc) case "diff-update-modify": scenarioDiffUpdateModify(enc) + case "untracked-file-setup": + scenarioUntrackedFileSetup(enc) + case "untracked-file-modify": + scenarioUntrackedFileModify(enc) case "clarification": scenarioClarification(enc) case "clarification-timeout": scenarioClarificationTimeout(enc) default: - emitTextBlock(enc, "Unknown e2e scenario: "+name+". Available: simple-message, read-and-edit, permission-flow, error, subagent, all-tools, multi-turn, diff-expansion-setup, diff-update-setup, diff-update-modify, clarification, clarification-timeout", "") + emitTextBlock(enc, "Unknown e2e scenario: "+name+". Available: simple-message, read-and-edit, permission-flow, error, subagent, all-tools, multi-turn, diff-expansion-setup, diff-update-setup, diff-update-modify, untracked-file-setup, untracked-file-modify, clarification, clarification-timeout", "") } } @@ -545,6 +549,44 @@ func scenarioDiffUpdateModify(enc *json.Encoder) { emitTextBlock(enc, "diff-update-modify complete: diff_update_test.txt now has SECOND_MODIFICATION", "") } +// scenarioUntrackedFileSetup creates a new untracked file (not staged, not committed). +// This tests that the UI shows diffs for untracked files. +func scenarioUntrackedFileSetup(enc *json.Encoder) { + fixedDelay(50) + + filePath := "untracked_test.txt" + content := "line 1: INITIAL_CONTENT\nline 2: some text\n" + + // Remove any existing file first + _ = os.Remove(filePath) + + // Write new untracked file + if err := os.WriteFile(filePath, []byte(content), 0o644); err != nil { + emitTextBlock(enc, "untracked-file-setup: write failed: "+err.Error(), "") + return + } + + fixedDelay(100) + emitTextBlock(enc, "untracked-file-setup complete: untracked_test.txt has INITIAL_CONTENT", "") +} + +// scenarioUntrackedFileModify modifies the untracked file created by untracked-file-setup. +// This tests that the UI updates the diff when an untracked file is modified. +func scenarioUntrackedFileModify(enc *json.Encoder) { + fixedDelay(50) + + filePath := "untracked_test.txt" + content := "line 1: MODIFIED_CONTENT\nline 2: some text\nline 3: NEW_LINE\n" + + if err := os.WriteFile(filePath, []byte(content), 0o644); err != nil { + emitTextBlock(enc, "untracked-file-modify: write failed: "+err.Error(), "") + return + } + + fixedDelay(100) + emitTextBlock(enc, "untracked-file-modify complete: untracked_test.txt now has MODIFIED_CONTENT", "") +} + // clarificationQuestionArgs returns the MCP arguments for the clarification question. func clarificationQuestionArgs() map[string]any { return map[string]any{ diff --git a/apps/backend/internal/agentctl/server/process/workspace_files.go b/apps/backend/internal/agentctl/server/process/workspace_files.go index 35e69b62f4..17556a42f0 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_files.go +++ b/apps/backend/internal/agentctl/server/process/workspace_files.go @@ -579,18 +579,19 @@ func (wt *WorkspaceTracker) GetFileContentAtRef(ctx context.Context, reqPath str gitRef := fmt.Sprintf("%s:%s", ref, cleanPath) // Preflight: check blob size before materializing content to avoid memory spikes. + // Use CombinedOutput to capture stderr for error detection (git writes errors to stderr). + // Set LC_ALL=C to ensure English error messages for reliable parsing. sizeCmd := exec.CommandContext(ctx, "git", "cat-file", "-s", gitRef) sizeCmd.Dir = wt.workDir - sizeOut, err := sizeCmd.Output() + sizeCmd.Env = append(os.Environ(), "LC_ALL=C") + sizeOut, err := sizeCmd.CombinedOutput() if err != nil { - if exitErr, ok := err.(*exec.ExitError); ok { - stderr := string(exitErr.Stderr) - if strings.Contains(stderr, "does not exist") || - strings.Contains(stderr, "Not a valid object") || - strings.Contains(stderr, "fatal: path") || - strings.Contains(stderr, "fatal: not a valid") { - return "", 0, false, fmt.Errorf("file not found at ref %s: %s", ref, cleanPath) - } + output := string(sizeOut) + if strings.Contains(output, "does not exist") || + strings.Contains(output, "Not a valid object") || + strings.Contains(output, "fatal: path") || + strings.Contains(output, "fatal: not a valid") { + return "", 0, false, fmt.Errorf("file not found at ref %s: %s", ref, cleanPath) } return "", 0, false, fmt.Errorf("failed to stat file at ref: %w", err) } diff --git a/apps/backend/internal/agentctl/server/process/workspace_monitor.go b/apps/backend/internal/agentctl/server/process/workspace_monitor.go index 5e72bc85ae..7f4553bd44 100644 --- a/apps/backend/internal/agentctl/server/process/workspace_monitor.go +++ b/apps/backend/internal/agentctl/server/process/workspace_monitor.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "os/exec" + "path/filepath" + "strings" "time" "github.com/kandev/kandev/internal/agentctl/types" @@ -61,11 +63,12 @@ func (wt *WorkspaceTracker) monitorLoop(ctx context.Context) { // workspaceState holds quick-check state for detecting workspace changes. type workspaceState struct { indexMtime time.Time // index mtime - changes on stage/unstage/commit - diffFilesID string // hash of git diff-files output - changes on any file modification + diffFilesID string // hash of git diff-files output - changes on tracked file modification + untrackedID string // hash of untracked files list + mtimes - changes on untracked file changes } // getWorkspaceState returns the current workspace state using fast checks. -// Uses index mtime + git diff-files output which includes file content hashes. +// Uses index mtime + git diff-files output + untracked file mtimes. func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) workspaceState { var state workspaceState @@ -87,12 +90,82 @@ func (wt *WorkspaceTracker) getWorkspaceState(ctx context.Context) workspaceStat state.diffFilesID = fmt.Sprintf("%d:%x:%x", len(out), out[0], out[len(out)-1]) } + // Check untracked files - git diff-files doesn't include them + // Use git ls-files to get untracked files, then check their mtimes + state.untrackedID = wt.getUntrackedFilesID(ctx) + return state } +// getUntrackedFilesID returns a hash identifying the current state of untracked files. +// Uses file list + mtimes to detect when untracked files are added, removed, or modified. +func (wt *WorkspaceTracker) getUntrackedFilesID(ctx context.Context) string { + // Get list of untracked files (excluding ignored) + cmd := exec.CommandContext(ctx, "git", "ls-files", "--others", "--exclude-standard") + cmd.Dir = wt.workDir + out, err := cmd.Output() + if err != nil || len(out) == 0 { + return "" + } + + // Build a simple hash from file paths + mtimes + // This is faster than hashing file contents + lines := strings.Split(strings.TrimSpace(string(out)), "\n") + var hashInput strings.Builder + for _, file := range lines { + if file == "" { + continue + } + hashInput.WriteString(file) + // Include mtime so we detect content changes + // Sanitize path to prevent directory traversal attacks (CodeQL security fix) + safePath, err := wt.sanitizePath(file) + if err != nil { + continue // Skip files with invalid paths + } + if info, err := os.Stat(safePath); err == nil { + hashInput.WriteString(fmt.Sprintf(":%d", info.ModTime().UnixNano())) + } + hashInput.WriteString(";") + } + + // Return length + first/last bytes as cheap identity (same pattern as diffFilesID) + s := hashInput.String() + if len(s) > 0 { + return fmt.Sprintf("%d:%x:%x", len(s), s[0], s[len(s)-1]) + } + return "" +} + +// sanitizePath validates that the given relative file path stays within the workspace directory. +// Returns the absolute path if valid, or an error if the path escapes the workspace. +func (wt *WorkspaceTracker) sanitizePath(relPath string) (string, error) { + // Clean the path to resolve any . or .. components + joined := filepath.Join(wt.workDir, relPath) + absPath, err := filepath.Abs(joined) + if err != nil { + return "", err + } + + // Ensure the resolved path is within the workspace directory + workDirAbs, err := filepath.Abs(wt.workDir) + if err != nil { + return "", err + } + + // Check that absPath starts with workDirAbs (with proper separator handling) + if !strings.HasPrefix(absPath, workDirAbs+string(os.PathSeparator)) && absPath != workDirAbs { + return "", fmt.Errorf("path escapes workspace: %s", relPath) + } + + return absPath, nil +} + // changed returns true if the workspace state has changed. func (s workspaceState) changed(other workspaceState) bool { - return s.indexMtime != other.indexMtime || s.diffFilesID != other.diffFilesID + return s.indexMtime != other.indexMtime || + s.diffFilesID != other.diffFilesID || + s.untrackedID != other.untrackedID } // emitFileChanges sends accumulated file changes to subscribers. diff --git a/apps/web/e2e/tests/diff-update.spec.ts b/apps/web/e2e/tests/diff-update.spec.ts index 4fe952e5c4..996f15b0ff 100644 --- a/apps/web/e2e/tests/diff-update.spec.ts +++ b/apps/web/e2e/tests/diff-update.spec.ts @@ -4,24 +4,32 @@ import type { ApiClient } from "../helpers/api-client"; import type { SeedData } from "../fixtures/test-base"; import type { Page } from "@playwright/test"; +// Shared selector constant for diff container +const DIFFS_CONTAINER = "diffs-container"; + +/** Options for seeding a task with an agent. */ +type SeedTaskOptions = { + title: string; + scenarioCommand: string; + completionText: string; +}; + /** - * Seed a task using the diff-update-setup mock scenario and navigate to + * Generic helper to seed a task using a mock scenario and navigate to * its session page, waiting for the agent turn to complete. - * - * The scenario creates a simple text file, commits it, then modifies line 1 - * to contain "FIRST_MODIFICATION", leaving an uncommitted diff. */ -async function seedDiffUpdateTask( +async function seedTaskWithScenario( testPage: Page, apiClient: ApiClient, seedData: SeedData, + options: SeedTaskOptions, ): Promise<{ session: SessionPage; sessionId: string }> { const task = await apiClient.createTaskWithAgent( seedData.workspaceId, - "Diff Update E2E", + options.title, seedData.agentProfileId, { - description: "/e2e:diff-update-setup", + description: options.scenarioCommand, workflow_id: seedData.workflowId, workflow_step_id: seedData.startStepId, repository_ids: [seedData.repositoryId], @@ -36,13 +44,31 @@ async function seedDiffUpdateTask( await session.waitForLoad(); // Wait for the first turn to complete - await expect(session.chat.getByText("diff-update-setup complete", { exact: false })).toBeVisible({ + await expect(session.chat.getByText(options.completionText, { exact: false })).toBeVisible({ timeout: 45_000, }); return { session, sessionId: task.session_id }; } +/** Seed a task for untracked file testing. */ +function seedUntrackedFileTask(testPage: Page, apiClient: ApiClient, seedData: SeedData) { + return seedTaskWithScenario(testPage, apiClient, seedData, { + title: "Untracked File E2E", + scenarioCommand: "/e2e:untracked-file-setup", + completionText: "untracked-file-setup complete", + }); +} + +/** Seed a task for diff update testing. */ +function seedDiffUpdateTask(testPage: Page, apiClient: ApiClient, seedData: SeedData) { + return seedTaskWithScenario(testPage, apiClient, seedData, { + title: "Diff Update E2E", + scenarioCommand: "/e2e:diff-update-setup", + completionText: "diff-update-setup complete", + }); +} + /** Click the Changes dockview tab. */ async function openChangesTab(testPage: Page) { const changesTab = testPage.locator(".dv-default-tab", { hasText: "Changes" }); @@ -50,26 +76,31 @@ async function openChangesTab(testPage: Page) { await changesTab.click(); } -/** Click the file row for diff_update_test.txt to open its diff view. */ -async function openDiffUpdateFileDiff(testPage: Page) { +/** Click a file row by name to open its diff view. */ +async function openFileDiff(testPage: Page, fileName: string) { const fileRow = testPage .locator("button, [role='button'], [class*='file']") - .filter({ hasText: "diff_update_test.txt" }) + .filter({ hasText: fileName }) .first(); await expect(fileRow).toBeVisible({ timeout: 10_000 }); await fileRow.click(); } +/** Helper to get the diffs container locator. */ +function getDiffsContainer(testPage: Page) { + return testPage.locator(DIFFS_CONTAINER); +} + test.describe("Diff update on file change", () => { test.describe.configure({ retries: 2, timeout: 120_000 }); test("shows initial diff with FIRST_MODIFICATION", async ({ testPage, apiClient, seedData }) => { await seedDiffUpdateTask(testPage, apiClient, seedData); await openChangesTab(testPage); - await openDiffUpdateFileDiff(testPage); + await openFileDiff(testPage, "diff_update_test.txt"); // The Pierre Diffs viewer should show the initial modification - const diffsContainer = testPage.locator("diffs-container"); + const diffsContainer = getDiffsContainer(testPage); await expect(diffsContainer).toBeVisible({ timeout: 15_000 }); await expect(diffsContainer.getByText("FIRST_MODIFICATION", { exact: true })).toBeVisible({ timeout: 15_000, @@ -79,10 +110,10 @@ test.describe("Diff update on file change", () => { test("diff updates when agent modifies file again", async ({ testPage, apiClient, seedData }) => { const { session } = await seedDiffUpdateTask(testPage, apiClient, seedData); await openChangesTab(testPage); - await openDiffUpdateFileDiff(testPage); + await openFileDiff(testPage, "diff_update_test.txt"); // Verify initial diff content (scoped to diffs-container to avoid matching chat text) - const diffsContainer = testPage.locator("diffs-container"); + const diffsContainer = getDiffsContainer(testPage); await expect(diffsContainer).toBeVisible({ timeout: 15_000 }); await expect(diffsContainer.getByText("FIRST_MODIFICATION", { exact: true })).toBeVisible({ timeout: 15_000, @@ -103,10 +134,10 @@ test.describe("Diff update on file change", () => { // The git status (with diff data) should have been updated via polling when // the file changed - this is the bug we're testing for. await openChangesTab(testPage); - await openDiffUpdateFileDiff(testPage); + await openFileDiff(testPage, "diff_update_test.txt"); // Re-query the diffs container since the DOM may have changed after tab switch - const updatedDiffsContainer = testPage.locator("diffs-container"); + const updatedDiffsContainer = getDiffsContainer(testPage); await expect(updatedDiffsContainer).toBeVisible({ timeout: 15_000 }); // The diff should now show SECOND_MODIFICATION instead of FIRST_MODIFICATION. @@ -126,3 +157,61 @@ test.describe("Diff update on file change", () => { }); }); }); + +test.describe("Untracked file diff update", () => { + test.describe.configure({ retries: 2, timeout: 120_000 }); + + test("untracked file diff updates when modified", async ({ testPage, apiClient, seedData }) => { + // This test verifies that modifying an untracked file triggers a git status update + // and the diff viewer shows the updated content. This was a bug where the polling + // mechanism didn't detect untracked file changes (git diff-files only shows tracked files). + const { session } = await seedUntrackedFileTask(testPage, apiClient, seedData); + await openChangesTab(testPage); + await openFileDiff(testPage, "untracked_test.txt"); + + // Verify initial diff content shows INITIAL_CONTENT + // Note: exact match is false because the line shows "line 1: INITIAL_CONTENT" + const diffsContainer = getDiffsContainer(testPage); + await expect(diffsContainer).toBeVisible({ timeout: 15_000 }); + await expect(diffsContainer.getByText("INITIAL_CONTENT")).toBeVisible({ + timeout: 15_000, + }); + + // Click on the Agent tab to make the chat input visible again + await session.clickTab("Agent"); + + // Send another message to trigger the modification + await session.sendMessage("/e2e:untracked-file-modify"); + + // Wait for the second turn to complete + await expect( + session.chat.getByText("untracked-file-modify complete", { exact: false }), + ).toBeVisible({ timeout: 45_000 }); + + // Wait for git polling to detect the file change (polling interval is ~1-2s) + await testPage.waitForTimeout(3_000); + + // Switch back to Changes tab and click on the diff file again + await openChangesTab(testPage); + await openFileDiff(testPage, "untracked_test.txt"); + + // Re-query the diffs container + const updatedDiffsContainer = getDiffsContainer(testPage); + await expect(updatedDiffsContainer).toBeVisible({ timeout: 15_000 }); + + // The diff should now show MODIFIED_CONTENT instead of INITIAL_CONTENT + // Note: exact match is false because the line includes prefix text + // Give extra time for git polling to detect and refresh the diff view + await expect(updatedDiffsContainer.getByText("MODIFIED_CONTENT")).toBeVisible({ + timeout: 45_000, + }); + + // Verify INITIAL_CONTENT is no longer shown + await expect(updatedDiffsContainer.getByText("INITIAL_CONTENT")).toHaveCount(0); + + // Also verify the new line was added + await expect(updatedDiffsContainer.getByText("NEW_LINE")).toBeVisible({ + timeout: 15_000, + }); + }); +});