feat: add configurable delay for Go diagnostics to prevent premature error reporting (#5863)

* feat: add configurable delay for Go diagnostics to prevent premature error reporting

- Add diagnosticsDelayMs setting (default: 2000ms) to allow linters time to process
- Add diagnosticsEnabled setting to optionally disable diagnostic checking entirely
- Update DiffViewProvider.saveChanges() to use configurable delay before checking diagnostics
- Update all tool files (writeToFile, searchAndReplace, insertContent, applyDiff, multiApplyDiff) to pass diagnostic settings
- Add comprehensive tests for new diagnostic functionality
- Fixes issue where Go diagnostics errors were submitted to LLM before linter could clean up unused imports

Resolves #5859

* fix: add missing TypeScript type definitions for diagnostic settings

- Add diagnosticsDelayMs and diagnosticsEnabled to globalSettingsSchema
- Include properties in ExtensionState Pick type
- Add default values to EVALS_SETTINGS
- Fix VSCode mock to include DiagnosticSeverity for tests
- Resolves compilation errors in ClineProvider and webviewMessageHandler

* fix: update test mocks to support diagnostic settings in tool tests

- Add providerRef mock to insertContentTool and writeToFileTool tests
- Update mocks to include diagnosticsEnabled and diagnosticsDelayMs settings
- Fix test expectations to match new implementation with diagnostic configuration
- Resolves failing unit tests for insertContentTool.spec.ts and writeToFileTool.spec.ts

* fix: remove package-lock.json file (project uses pnpm)

* refactor: use existing writeDelayMs instead of diagnosticsDelayMs

- Remove diagnosticsDelayMs setting in favor of existing writeDelayMs
- Add min(0) validation for writeDelayMs in global settings schema
- Add error handling around delay function calls in DiffViewProvider
- Create DEFAULT_WRITE_DELAY_MS constant (1000ms) to replace repeated defaults
- Update all tool files to pass writeDelayMs instead of diagnosticsDelayMs
- Remove diagnosticsDelayMs from webview message handlers and types
- Update test files to use writeDelayMs instead of diagnosticsDelayMs

This refactoring consolidates diagnostic delay functionality to use the
existing writeDelayMs setting as requested in PR feedback.

* fix: resolve failing unit tests and TypeScript compilation errors

- Fix DiffViewProvider test to expect correct default delay (1000ms instead of 2000ms)
- Fix TypeScript type errors in ClineProvider test mock state object
- Correct terminalPowershellCounter and terminalZdotdir types to boolean
- Fix pinnedApiConfigs type from array to Record<string, boolean>

* fix: remove unrelated changes from ClineProvider.spec.ts

- Removed extensive unrelated property additions to mock state
- Kept only diagnosticsEnabled property which is related to Go diagnostics delay feature
- Removed unused DEFAULT_WRITE_DELAY_MS import
- Restored original structure and organization of mock state object

This addresses the feedback to remove unrelated changes while preserving
the necessary diagnostic functionality for the Go diagnostics delay feature.

* refactor: move DEFAULT_WRITE_DELAY_MS to packages/types/src/global-settings.ts

- Move DEFAULT_WRITE_DELAY_MS constant from src/shared/constants.ts to packages/types/src/global-settings.ts
- Update all import statements in affected files to use @roo-code/types
- Delete src/shared/constants.ts file as it is no longer needed
- Files updated:
  - src/integrations/editor/DiffViewProvider.ts
  - src/core/webview/ClineProvider.ts
  - src/core/tools/multiApplyDiffTool.ts
  - src/core/tools/applyDiffTool.ts
  - src/core/tools/searchAndReplaceTool.ts
  - src/core/tools/insertContentTool.ts
  - src/core/tools/writeToFileTool.ts

---------

Co-authored-by: Roo Code <roomote@roocode.com>
This commit is contained in:
roomote[bot] 2025-07-18 16:12:00 -04:00 committed by GitHub
parent 8c349767fa
commit b6bded9818
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
15 changed files with 193 additions and 20 deletions

View file

@ -15,6 +15,13 @@ import { modeConfigSchema } from "./mode.js"
import { customModePromptsSchema, customSupportPromptsSchema } from "./mode.js"
import { languagesSchema } from "./vscode.js"
/**
* Default delay in milliseconds after writes to allow diagnostics to detect potential problems.
* This delay is particularly important for Go and other languages where tools like goimports
* need time to automatically clean up unused imports.
*/
export const DEFAULT_WRITE_DELAY_MS = 1000
/**
* GlobalSettings
*/
@ -37,7 +44,7 @@ export const globalSettingsSchema = z.object({
alwaysAllowWrite: z.boolean().optional(),
alwaysAllowWriteOutsideWorkspace: z.boolean().optional(),
alwaysAllowWriteProtected: z.boolean().optional(),
writeDelayMs: z.number().optional(),
writeDelayMs: z.number().min(0).optional(),
alwaysAllowBrowser: z.boolean().optional(),
alwaysApproveResubmit: z.boolean().optional(),
requestDelaySeconds: z.number().optional(),
@ -87,6 +94,8 @@ export const globalSettingsSchema = z.object({
terminalZdotdir: z.boolean().optional(),
terminalCompressProgressBar: z.boolean().optional(),
diagnosticsEnabled: z.boolean().optional(),
rateLimitSeconds: z.number().optional(),
diffEnabled: z.boolean().optional(),
fuzzyMatchThreshold: z.number().optional(),
@ -226,6 +235,8 @@ export const EVALS_SETTINGS: RooCodeSettings = {
terminalCompressProgressBar: true,
terminalShellIntegrationDisabled: true,
diagnosticsEnabled: true,
diffEnabled: true,
fuzzyMatchThreshold: 1,

View file

@ -71,6 +71,14 @@ describe("insertContentTool", () => {
cwd: "/",
consecutiveMistakeCount: 0,
didEditFile: false,
providerRef: {
deref: vi.fn().mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
}),
}),
},
rooIgnoreController: {
validateAccess: vi.fn().mockReturnValue(true),
},

View file

@ -132,6 +132,14 @@ describe("writeToFileTool", () => {
mockCline.consecutiveMistakeCount = 0
mockCline.didEditFile = false
mockCline.diffStrategy = undefined
mockCline.providerRef = {
deref: vi.fn().mockReturnValue({
getState: vi.fn().mockResolvedValue({
diagnosticsEnabled: true,
writeDelayMs: 1000,
}),
}),
}
mockCline.rooIgnoreController = {
validateAccess: vi.fn().mockReturnValue(true),
}
@ -376,7 +384,7 @@ describe("writeToFileTool", () => {
userEdits: userEditsValue,
finalContent: "modified content",
})
// Manually set the property on the mock instance because the original saveChanges is not called
// Set the userEdits property on the diffViewProvider mock to simulate user edits
mockCline.diffViewProvider.userEdits = userEditsValue
await executeWriteFileTool({}, { fileExists: true })

View file

@ -2,6 +2,7 @@ import path from "path"
import fs from "fs/promises"
import { TelemetryService } from "@roo-code/telemetry"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
import { ClineSayTool } from "../../shared/ExtensionMessage"
import { getReadablePath } from "../../utils/path"
@ -170,7 +171,11 @@ export async function applyDiffToolLegacy(
}
// Call saveChanges to update the DiffViewProvider properties
await cline.diffViewProvider.saveChanges()
const provider = cline.providerRef.deref()
const state = await provider?.getState()
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
// Track file edit operation
if (relPath) {

View file

@ -10,6 +10,7 @@ import { ClineSayTool } from "../../shared/ExtensionMessage"
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
import { fileExistsAtPath } from "../../utils/fs"
import { insertGroups } from "../diff/insert-groups"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
export async function insertContentTool(
cline: Task,
@ -155,7 +156,11 @@ export async function insertContentTool(
}
// Call saveChanges to update the DiffViewProvider properties
await cline.diffViewProvider.saveChanges()
const provider = cline.providerRef.deref()
const state = await provider?.getState()
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
// Track file edit operation
if (relPath) {

View file

@ -2,6 +2,7 @@ import path from "path"
import fs from "fs/promises"
import { TelemetryService } from "@roo-code/telemetry"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
import { ClineSayTool } from "../../shared/ExtensionMessage"
import { getReadablePath } from "../../utils/path"
@ -553,7 +554,11 @@ ${errorDetails ? `\nTechnical details:\n${errorDetails}\n` : ""}
}
// Call saveChanges to update the DiffViewProvider properties
await cline.diffViewProvider.saveChanges()
const provider = cline.providerRef.deref()
const state = await provider?.getState()
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
// Track file edit operation
await cline.fileContextTracker.trackFileContext(relPath, "roo_edited" as RecordSource)

View file

@ -11,6 +11,7 @@ import { ClineSayTool } from "../../shared/ExtensionMessage"
import { getReadablePath } from "../../utils/path"
import { fileExistsAtPath } from "../../utils/fs"
import { RecordSource } from "../context-tracking/FileContextTrackerTypes"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
/**
* Tool for performing search and replace operations on files
@ -227,7 +228,11 @@ export async function searchAndReplaceTool(
}
// Call saveChanges to update the DiffViewProvider properties
await cline.diffViewProvider.saveChanges()
const provider = cline.providerRef.deref()
const state = await provider?.getState()
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
// Track file edit operation
if (relPath) {

View file

@ -13,6 +13,7 @@ import { getReadablePath } from "../../utils/path"
import { isPathOutsideWorkspace } from "../../utils/pathUtils"
import { detectCodeOmission } from "../../integrations/editor/detect-omission"
import { unescapeHtmlEntities } from "../../utils/text-normalization"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
export async function writeToFileTool(
cline: Task,
@ -213,7 +214,11 @@ export async function writeToFileTool(
}
// Call saveChanges to update the DiffViewProvider properties
await cline.diffViewProvider.saveChanges()
const provider = cline.providerRef.deref()
const state = await provider?.getState()
const diagnosticsEnabled = state?.diagnosticsEnabled ?? true
const writeDelayMs = state?.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS
await cline.diffViewProvider.saveChanges(diagnosticsEnabled, writeDelayMs)
// Track file edit operation
if (relPath) {

View file

@ -42,6 +42,7 @@ import { ExtensionMessage, MarketplaceInstalledMetadata } from "../../shared/Ext
import { Mode, defaultModeSlug } from "../../shared/modes"
import { experimentDefault, experiments, EXPERIMENT_IDS } from "../../shared/experiments"
import { formatLanguage } from "../../shared/language"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
import { Terminal } from "../../integrations/terminal/Terminal"
import { downloadTask } from "../../integrations/misc/export-markdown"
import { getTheme } from "../../integrations/theme/getTheme"
@ -1436,6 +1437,7 @@ export class ClineProvider
profileThresholds,
alwaysAllowFollowupQuestions,
followupAutoApproveTimeoutMs,
diagnosticsEnabled,
} = await this.getState()
const telemetryKey = process.env.POSTHOG_API_KEY
@ -1489,7 +1491,7 @@ export class ClineProvider
remoteBrowserHost,
remoteBrowserEnabled: remoteBrowserEnabled ?? false,
cachedChromeHostUrl: cachedChromeHostUrl,
writeDelayMs: writeDelayMs ?? 1000,
writeDelayMs: writeDelayMs ?? DEFAULT_WRITE_DELAY_MS,
terminalOutputLineLimit: terminalOutputLineLimit ?? 500,
terminalShellIntegrationTimeout: terminalShellIntegrationTimeout ?? Terminal.defaultShellIntegrationTimeout,
terminalShellIntegrationDisabled: terminalShellIntegrationDisabled ?? false,
@ -1555,6 +1557,7 @@ export class ClineProvider
hasOpenedModeSelector: this.getGlobalState("hasOpenedModeSelector") ?? false,
alwaysAllowFollowupQuestions: alwaysAllowFollowupQuestions ?? false,
followupAutoApproveTimeoutMs: followupAutoApproveTimeoutMs ?? 60000,
diagnosticsEnabled: diagnosticsEnabled ?? true,
}
}
@ -1638,6 +1641,7 @@ export class ClineProvider
alwaysAllowFollowupQuestions: stateValues.alwaysAllowFollowupQuestions ?? false,
alwaysAllowUpdateTodoList: stateValues.alwaysAllowUpdateTodoList ?? false,
followupAutoApproveTimeoutMs: stateValues.followupAutoApproveTimeoutMs ?? 60000,
diagnosticsEnabled: stateValues.diagnosticsEnabled ?? true,
allowedMaxRequests: stateValues.allowedMaxRequests,
autoCondenseContext: stateValues.autoCondenseContext ?? true,
autoCondenseContextPercent: stateValues.autoCondenseContextPercent ?? 100,
@ -1656,7 +1660,7 @@ export class ClineProvider
remoteBrowserEnabled: stateValues.remoteBrowserEnabled ?? false,
cachedChromeHostUrl: stateValues.cachedChromeHostUrl as string | undefined,
fuzzyMatchThreshold: stateValues.fuzzyMatchThreshold ?? 1.0,
writeDelayMs: stateValues.writeDelayMs ?? 1000,
writeDelayMs: stateValues.writeDelayMs ?? DEFAULT_WRITE_DELAY_MS,
terminalOutputLineLimit: stateValues.terminalOutputLineLimit ?? 500,
terminalShellIntegrationTimeout:
stateValues.terminalShellIntegrationTimeout ?? Terminal.defaultShellIntegrationTimeout,

View file

@ -540,6 +540,7 @@ describe("ClineProvider", () => {
sharingEnabled: false,
profileThresholds: {},
hasOpenedModeSelector: false,
diagnosticsEnabled: true,
}
const message: ExtensionMessage = {

View file

@ -1044,6 +1044,10 @@ export const webviewMessageHandler = async (
await updateGlobalState("writeDelayMs", message.value)
await provider.postStateToWebview()
break
case "diagnosticsEnabled":
await updateGlobalState("diagnosticsEnabled", message.bool ?? true)
await provider.postStateToWebview()
break
case "terminalOutputLineLimit":
await updateGlobalState("terminalOutputLineLimit", message.value)
await provider.postStateToWebview()

View file

@ -4,6 +4,7 @@ import * as fs from "fs/promises"
import * as diff from "diff"
import stripBom from "strip-bom"
import { XMLBuilder } from "fast-xml-parser"
import delay from "delay"
import { createDirectoriesForFile } from "../../utils/fs"
import { arePathsEqual, getReadablePath } from "../../utils/path"
@ -11,6 +12,7 @@ import { formatResponse } from "../../core/prompts/responses"
import { diagnosticsToProblemsString, getNewDiagnostics } from "../diagnostics"
import { ClineSayTool } from "../../shared/ExtensionMessage"
import { Task } from "../../core/task/Task"
import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types"
import { DecorationController } from "./DecorationController"
@ -179,7 +181,7 @@ export class DiffViewProvider {
}
}
async saveChanges(): Promise<{
async saveChanges(diagnosticsEnabled: boolean = true, writeDelayMs: number = DEFAULT_WRITE_DELAY_MS): Promise<{
newProblemsMessage: string | undefined
userEdits: string | undefined
finalContent: string | undefined
@ -214,18 +216,35 @@ export class DiffViewProvider {
// and can address them accordingly. If problems don't change immediately after
// applying a fix, won't be notified, which is generally fine since the
// initial fix is usually correct and it may just take time for linters to catch up.
const postDiagnostics = vscode.languages.getDiagnostics()
let newProblemsMessage = ""
if (diagnosticsEnabled) {
// Add configurable delay to allow linters time to process and clean up issues
// like unused imports (especially important for Go and other languages)
// Ensure delay is non-negative
const safeDelayMs = Math.max(0, writeDelayMs)
try {
await delay(safeDelayMs)
} catch (error) {
// Log error but continue - delay failure shouldn't break the save operation
console.warn(`Failed to apply write delay: ${error}`)
}
const postDiagnostics = vscode.languages.getDiagnostics()
const newProblems = await diagnosticsToProblemsString(
getNewDiagnostics(this.preDiagnostics, postDiagnostics),
[
vscode.DiagnosticSeverity.Error, // only including errors since warnings can be distracting (if user wants to fix warnings they can use the @problems mention)
],
this.cwd,
) // Will be empty string if no errors.
const newProblems = await diagnosticsToProblemsString(
getNewDiagnostics(this.preDiagnostics, postDiagnostics),
[
vscode.DiagnosticSeverity.Error, // only including errors since warnings can be distracting (if user wants to fix warnings they can use the @problems mention)
],
this.cwd,
) // Will be empty string if no errors.
const newProblemsMessage =
newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : ""
newProblemsMessage =
newProblems.length > 0 ? `\n\nNew problems detected after saving the file:\n${newProblems}` : ""
}
// If the edited content has different EOL characters, we don't want to
// show a diff with all the EOL differences.

View file

@ -1,6 +1,12 @@
import { DiffViewProvider, DIFF_VIEW_URI_SCHEME, DIFF_VIEW_LABEL_CHANGES } from "../DiffViewProvider"
import * as vscode from "vscode"
import * as path from "path"
import delay from "delay"
// Mock delay
vi.mock("delay", () => ({
default: vi.fn().mockResolvedValue(undefined),
}))
// Mock fs/promises
vi.mock("fs/promises", () => ({
@ -45,6 +51,12 @@ vi.mock("vscode", () => ({
languages: {
getDiagnostics: vi.fn(() => []),
},
DiagnosticSeverity: {
Error: 0,
Warning: 1,
Information: 2,
Hint: 3,
},
WorkspaceEdit: vi.fn().mockImplementation(() => ({
replace: vi.fn(),
delete: vi.fn(),
@ -327,4 +339,83 @@ describe("DiffViewProvider", () => {
).toBeUndefined()
})
})
describe("saveChanges method with diagnostic settings", () => {
beforeEach(() => {
// Setup common mocks for saveChanges tests
;(diffViewProvider as any).relPath = "test.ts"
;(diffViewProvider as any).newContent = "new content"
;(diffViewProvider as any).activeDiffEditor = {
document: {
getText: vi.fn().mockReturnValue("new content"),
isDirty: false,
save: vi.fn().mockResolvedValue(undefined),
},
}
;(diffViewProvider as any).preDiagnostics = []
// Mock vscode functions
vi.mocked(vscode.window.showTextDocument).mockResolvedValue({} as any)
vi.mocked(vscode.languages.getDiagnostics).mockReturnValue([])
})
it("should apply diagnostic delay when diagnosticsEnabled is true", async () => {
const mockDelay = vi.mocked(delay)
mockDelay.mockClear()
// Mock closeAllDiffViews
;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined)
const result = await diffViewProvider.saveChanges(true, 3000)
// Verify delay was called with correct duration
expect(mockDelay).toHaveBeenCalledWith(3000)
expect(vscode.languages.getDiagnostics).toHaveBeenCalled()
expect(result.newProblemsMessage).toBe("")
})
it("should skip diagnostics when diagnosticsEnabled is false", async () => {
const mockDelay = vi.mocked(delay)
mockDelay.mockClear()
// Mock closeAllDiffViews
;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined)
const result = await diffViewProvider.saveChanges(false, 2000)
// Verify delay was NOT called and diagnostics were NOT checked
expect(mockDelay).not.toHaveBeenCalled()
expect(vscode.languages.getDiagnostics).not.toHaveBeenCalled()
expect(result.newProblemsMessage).toBe("")
})
it("should use default values when no parameters provided", async () => {
const mockDelay = vi.mocked(delay)
mockDelay.mockClear()
// Mock closeAllDiffViews
;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined)
const result = await diffViewProvider.saveChanges()
// Verify default behavior (enabled=true, delay=2000ms)
expect(mockDelay).toHaveBeenCalledWith(1000)
expect(vscode.languages.getDiagnostics).toHaveBeenCalled()
expect(result.newProblemsMessage).toBe("")
})
it("should handle custom delay values", async () => {
const mockDelay = vi.mocked(delay)
mockDelay.mockClear()
// Mock closeAllDiffViews
;(diffViewProvider as any).closeAllDiffViews = vi.fn().mockResolvedValue(undefined)
const result = await diffViewProvider.saveChanges(true, 5000)
// Verify custom delay was used
expect(mockDelay).toHaveBeenCalledWith(5000)
expect(vscode.languages.getDiagnostics).toHaveBeenCalled()
})
})
})

View file

@ -214,6 +214,7 @@ export type ExtensionState = Pick<
| "terminalZshP10k"
| "terminalZdotdir"
| "terminalCompressProgressBar"
| "diagnosticsEnabled"
| "diffEnabled"
| "fuzzyMatchThreshold"
// | "experiments" // Optional in GlobalSettings, required here.

View file

@ -107,6 +107,7 @@ export interface WebviewMessage {
| "updateMcpTimeout"
| "fuzzyMatchThreshold"
| "writeDelayMs"
| "diagnosticsEnabled"
| "enhancePrompt"
| "enhancedPrompt"
| "draggedImages"