-
Notifications
You must be signed in to change notification settings - Fork 373
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
kill RemoteSearch, unify context fetching paths #5379
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sqs
approved these changes
Aug 29, 2024
e05e49a
to
bc3c60f
Compare
janhartman
approved these changes
Aug 29, 2024
vovakulikov
added a commit
that referenced
this pull request
Sep 3, 2024
This PR fixes regression after #5379 In #5379 remote repository context logic was removed, which was important for Cody Web. When you start typing `@<mention-term>` by default, we do a file mention search; in Cody Web world, we rely on remote context repository API to look for files within these repositories from remote context API. This is convenient when you open Cody Web on the repository page, and automatically, your file and symbol mentions will be scoped to this repo. Since this context remote repository has been removed this PR passes remote repositories info for mentions directly through API (it's even better because we don't rely on the internal chat controller state anymore) Note that this change has no effect on VSCode and only fixes logic for Cody Web ## Test plan - Run Cody Web demo - Check that file mentions and symbol mentions are scoped to the cody repository (we use it as scope repository in the demo)
olafurpg
added a commit
that referenced
this pull request
Sep 9, 2024
Fixes CODY-3687 Previously, `cody chat --context-repo REPO` didn't use the provided repo as context due to a regresion in the PR #5379 This bug was limited to version 5.5.14 of the CLI, it was not part of 5.5.13. This PR fixes the bug by adding repo context to the `'submit'` `contextItem` property where we previously only sent files.
olafurpg
added a commit
that referenced
this pull request
Sep 9, 2024
Fixes CODY-3687 Previously, `cody chat --context-repo REPO` didn't use the provided repo as context due to a regresion in the PR #5379 This bug was limited to version 5.5.14 of the CLI, it was not part of 5.5.13. This PR fixes the bug by adding repo context to the `'submit'` `contextItem` property where we previously only sent files.
olafurpg
added a commit
that referenced
this pull request
Sep 9, 2024
Fixes CODY-3687 Previously, `cody chat --context-repo REPO` didn't use the provided repo as context due to a regresion in the PR #5379 This bug was limited to version 5.5.14 of the CLI, it was not part of 5.5.13. This PR fixes the bug by adding repo context to the `'submit'` `contextItem` property where we previously only sent files. ## Test plan Manually tested with this command ``` ❯ pnpm agent chat --show-context --context-file agent/README.md --context-repo github.com/sourcegraph/cody -m 'what is the agent?' ... > Context items: > 1. /Users/olafurpg/dev/sourcegraph/cody/agent/agent/README.md > 2. https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody/-/blob/agent/src/__tests__/graph-test/README.md > 3. https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody/-/blob/cli/README.md > 4. https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody/-/blob/agent/src/local-e2e/README.md ... ``` We have an integration test for `--context-repo` but it got disabled a couple months ago due to it being flaky. <!-- Required. See https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles. --> ## Changelog <!-- OPTIONAL; info at https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Kills off RemoteSearch and separate code path for enterprise context. Also kills off the legacy context for PLG. Both are replaced by a unified context-fetching path that runs through the
ContextRetriever
class.This maintains behavioral parity, as verified by this spreadsheet: https://docs.google.com/spreadsheets/d/1DWxPW6ktY4XgM_IEBw10hjpI00mqcv08-APOtpNfaAY/edit
This also inadvertently fixes a bug where we can return empty context if we receive a context result from the server with a negative end line.
Test plan
Run against s2 and dotcom with new and old context with example queries.