Skip to content
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

feat: filter context with .cody/.ignore #1382

Merged
merged 35 commits into from
Dec 29, 2023
Merged

feat: filter context with .cody/.ignore #1382

merged 35 commits into from
Dec 29, 2023

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Oct 12, 2023

CLOSE #1049

For details, please see RFC 852 File Exclusion Patterns for Cody

feat: filter context from ignored files

IMPORTANT: Currently behind the cody.internal.unstable config and must be enabled to try the feature. When this is out of internal experimental stage, this should be enabled by default.

  • Add isCodyIgnoreFile utility to check if file is defined in .cody/.ignore (at workspace level)
  • Update Interaction to filter context messages from ignored files when first created
  • Update context gathering in completions to skip ignored files
  • Add check in editor to skip ignored files
  • ignore all .env files / path by default

All clients should provide file content from the .cody/.ignore file at start up, and update the ignoreList when the file is changed.

Note

This PR includes update to the VS Code clients so that the ignore list is set at Editor start up, and whenever the .cody/.ignore file is updated, the ignore list will be updated accordingly.

This does not make any changes to the current embedding steps or BFG, which will be a feature to be implemented in the future as listed by Dom in the discussion below.

  • PRFAQ to agree on the .cody/.ignorename

Test plan

Prep:

image
  • Build Cody from this branch, start in VS Code debug mode
  • Turn on the Unstable Experimental Features from the Cody Settings
  • Open the cody repository
  • Create a .codyignore file
  • Add the following to the .codyignore file
vscode/**
utils.ts

Chat

Ask cody about fixup in Chat View: "what is fixup?"

You should see Cody did not read any files from the vscode directory to answer your question:

image

Autocomplete

Go to any of the file inside the vscode directory, you should not get any autocomplete suggestions.

image

Command

Go to lib/shared/src/utils.ts file, and then select some code, right click and select Explain Code:
image

You should see cody explain a random code (hallucinating because we didn't provide any code as context) that is not your selected code.

image

You can check the output channel to confirm no context was sent to the LLM:

image

Demo

DEMO_.cody_.ignore.file.mp4

Update

Here is a loom video of codyignore working in Simple chat panel:

https://www.loom.com/share/83fe3e0b0728449385d130d45c317cfa

Copy link
Contributor

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Left some early comments but feel free to ignore

lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/transcript/interaction.ts Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.test.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
lib/shared/src/chat/context-filter.ts Outdated Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
@abeatrix
Copy link
Contributor Author

abeatrix commented Nov 3, 2023

@DanTup Thank you so much for reviewing this PR!

Updated the PR to address your very helpful feedback, especially the point regarding handling relative file paths in isCodyIgnoredFile:

  • Update isCodyIgnoredFile to handle relative file paths instead of absolute
  • Extract current ignore file directory and make file paths relative to it
  • Handle errors in isCodyIgnoredFile instead of returning false
  • Update setCodyIgnoreList docs to mention it takes ignore file path

I haven't looked into passing the entire ignore file which seems to make things a lot easier, will continue the discussion with you on Slack!

@abeatrix abeatrix requested review from chwarwick and a team November 15, 2023 18:24
@abeatrix abeatrix marked this pull request as ready for review November 15, 2023 18:26
@slimsag
Copy link
Member

slimsag commented Nov 15, 2023

Q:

IMPORTANT: Currently behind the cody.internal.unstable config and must be enabled to try the feature. When this is out of internal experimental stage, this should be enabled by default.

What's the motivation behind this vs. making the existence of a .cody/.ignore file opt-in to this functionality? When would it not be behind the feature flag?

/**
* The Cody ignore file path in POSIX style (always forward slashes).
*/
export const CODY_IGNORE_FILENAME_POSIX_GLOB = path.posix.join('**', '.cody', '.ignore')
Copy link
Member

Choose a reason for hiding this comment

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

I notice the PR title and description mentions .codyignore, while the code here seems to suggest .cody/.ignore. (not commenting on the awkwardness of two . files..)

which one is right / expected to work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll be .cody/.ignore! Will attach the RFC to the issue to clarify! RFC 852 File Exclusion Patterns for Cody

Copy link
Member

@slimsag slimsag left a comment

Choose a reason for hiding this comment

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

This is quite useful to us on the Cody strat team, thank you for working on this!

@abeatrix
Copy link
Contributor Author

Q:

IMPORTANT: Currently behind the cody.internal.unstable config and must be enabled to try the feature. When this is out of internal experimental stage, this should be enabled by default.

What's the motivation behind this vs. making the existence of a .cody/.ignore file opt-in to this functionality? When would it not be behind the feature flag?

I wanted to emphasize on the fact that this feature is still in experimental and unstable (not throughly tested so users must manually enable this feature to confirm before they can test it out.

I'm thinking once we have tested this internally and have .Cody/.ignore working for embedding jobs, we can have it enabled by default.

Do you think this is a good idea, or am I overthinking 😂?

Copy link
Contributor

@DanTup DanTup left a comment

Choose a reason for hiding this comment

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

LGTM with a few minor comments (although I'm less familiar with some of this code so you may want another reviewer too).

lib/shared/src/chat/transcript/interaction.ts Outdated Show resolved Hide resolved
vscode/src/repository/repositoryHelpers.ts Outdated Show resolved Hide resolved
vscode/src/repository/repositoryHelpers.ts Outdated Show resolved Hide resolved
vscode/src/services/context-filter.ts Outdated Show resolved Hide resolved
}
}

await extension?.activate().then(() => init())
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to worry about any exceptions here (for example if you disable the Git extension does it throw?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahh i added try catch to this for cases where Git extension is disabled. we probably will need to do some follow up works on UX improvement but I think what we have is a good start. I am going to do some final testings before I merge it but consider this is currently behind the cody.internal.unstable config flag, i think we should be good 🙏

Thank you so much for your help and your throughout review 🙇‍♀️

@abeatrix abeatrix merged commit c9a4f7e into main Dec 29, 2023
13 checks passed
@abeatrix abeatrix deleted the bee/codyignore branch December 29, 2023 21:15
@abeatrix abeatrix changed the title feat: filter context from .cody/.ignore feat: filter context with .cody/.ignore Jan 9, 2024
@abeatrix abeatrix self-assigned this Jan 9, 2024
@abeatrix abeatrix mentioned this pull request Jan 16, 2024
33 tasks
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.

Introduce exclusion patterns for files that shouldn't be sent over the network
6 participants