Skip to content

fix: diff reports no lines added or removed #1549

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

awschristou
Copy link
Contributor

Problem

When agentic chat modifies existing files, chat often reports the delta as +0 -0 (no file changes) on Windows systems.

image

This causes the following problems:

  • clicking on the diff shows the entire file as a new change, instead of just the changed contents
  • clicking Undo will delete the file, because chat thinks the file was newly added as a result of the modification

Causes

  • getTextDocument is called with a mixture of strings that contain uris and local filesystem paths (1, 2)
  • getTextDocument calls workspace.getTextDocument which is expecting a string in uri format. The calls made using local filesystem paths fail 100% of the time.
  • when calls to workspace.getTextDocument fail, getTextDocument falls back to directly reading from the filesystem, however it tries to do this by parsing the given input (which is sometimes a local file path) into a Uri. This frequently throws an error, which causes getTextDocument to return undefined. Calling code incorrectly assumes this means its working with a new file.
  • when getTextDocument is successful, the chat controller calls it again after modifying the file, which attempts to load from the LSP workspace (workspace.getTextDocument). There are times when the LSP workspace is not updated with the latest content, because some IDEs are slow to pick up changes from files altered outside of the IDE process. For example, some IDEs don't register file content changes when they do not have active focus until after the IDE receives focus again. As a result, the chat controller thinks the file contents before and after a change are the same.
  • on Windows, there is some variation in the URI used to store each file in the LSP workspace. For example, VS Code currently looks like file:///C%3A/Foo/bar.txt but Visual Studio looks like file:///C:/Foo/bar.txt. When requesting LSP workspace files, the URI must be exact, and is case sensitive.

Solution

  • separate the getTextDocument calls, so callers can explicitly request content using a uri (getTextDocumentFromUri) or a filepath (getTextDocumentFromPath)
  • when requesting a testDocument using a filepath on Windows, attempt to retrieve the document from the LSP workspace using a combination of different URI styles, and drive casings.
    • This increases the success rate obtaining the content from the LSP workspace.
    • This reduces the likelihood the system thinks the file is newly created
  • only fetch file contents from the filesystem (not the LSP workspace) after the fsWrite tool alters a file, so that calling code has the true file contents from after a file modification.
    • This reduces the likelihood the system thinks the file has not been modified

This change was tested in Visual Studio, and using the sample VS Code extension in this repo. In both cases, I was able to see the problem occur before my change, and disappear after my change.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@awschristou awschristou requested a review from a team as a code owner June 6, 2025 01:09
@@ -1003,7 +1003,7 @@ export class AgenticChatController implements ChatHandlers {

if (toolUse.name === 'fsWrite') {
const input = toolUse.input as unknown as FsWriteParams
const document = await this.#triggerContext.getTextDocument(input.path)
const document = await this.#triggerContext.getTextDocumentFromPath(input.path, true, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think even here we should only load from fs because edits requires fs content, not LSP content

@@ -242,7 +242,7 @@ export class AgenticChatTriggerContext {
if (textDocumentIdentifier?.uri === undefined) {
return
}
const textDocument = await this.getTextDocument(textDocumentIdentifier.uri)
const textDocument = await this.getTextDocumentFromUri(textDocumentIdentifier.uri)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we load from fs here as well to keep it consistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with most of this code flow. Do you know what the impact would be if we avoid using the workspace altogether?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about this further, I think we want both of the places you mentioned to continue consuming content from the workspace. I believe that if the user has unsaved edits in the document within the IDE, these edits are reflected in the LSP workspace copy, and that's what we'd want to use as the LLM context and as the fsWrite tool input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants