fix(cody): remove client check for context filters#63855
Conversation
3496c1b to
f4e978c
Compare
Previously, the `--context-repo` option did not work with the CLI because of two issues: - The CLI used the client name "cody-cli", which Sourcegraph Enterprise rejects due to a problem that is described in the PR https://github.com/sourcegraph/sourcegraph/pull/63855 To work around this problem, the CLI will use the client name "jetbrains" for time being. - The `addEnhancedOptions` was set to false in `chat/submitMessage`. We now set this option to `true` when `--context-repo` or `--context-file` is enabled.
Previously, the `--context-repo` option did not work with the CLI because of two issues: - The CLI used the client name "cody-cli", which Sourcegraph Enterprise rejects due to a problem that is described in the PR https://github.com/sourcegraph/sourcegraph/pull/63855 To work around this problem, the CLI will use the client name "jetbrains" for time being. - The `addEnhancedOptions` was set to false in `chat/submitMessage`. We now set this option to `true` when `--context-repo` or `--context-file` is enabled.
We have to bump the version to 5.5.9 to bypass the server checks that will (most likely) be removed in https://github.com/sourcegraph/sourcegraph/pull/63855
We have to bump the version to 5.5.9 to bypass the server checks that will (most likely) be removed in https://github.com/sourcegraph/sourcegraph/pull/63855
| @@ -83,10 +83,7 @@ func TestCheckClientCodyIgnoreCompatibility(t *testing.T) { | |||
| q: url.Values{ | |||
| "client-name": []string{"sublime"}, | |||
There was a problem hiding this comment.
we require both clientName and clientVersion params but check the version only for VS Code and JetBrains
This test still should fail based on this requirement (thread)
There was a problem hiding this comment.
Good catch. Fixed.
| // (JetBrains, Eclipse, Visual Studio) support context filters out of | ||
| // the box since the original support was added for JetBrains GA in May | ||
| // 2024. | ||
| return nil |
There was a problem hiding this comment.
We need to check if the client version is provided. If it's not, the function should still return an error (thread).
There was a problem hiding this comment.
Good catch. I've updated the logic to accept an empty version for web only, and require semver for other clients. Added relevant tests
6cd6ac5 to
2da103e
Compare
2da103e to
0ff6d30
Compare
|
Looks like the meat of this PR got merged via https://github.com/sourcegraph/sourcegraph/commit/277648821ccf929574b910ac027b1dfcf1235bf2 This PR now only includes changes to address the review comments |
| // All agent-based clients (JetBrains, Eclipse, Visual Studio) support | ||
| // context filters out of the box since the original support was added | ||
| // for JetBrains GA in May 2024. | ||
| cvc = clientVersionConstraint{client: clientName, constraint: ">= 0.0.0"} |
There was a problem hiding this comment.
We should also consider the value of the cody-context-filters-clients-test-mode feature flag (refer to the logic a few lines above). Pre-release versions might not satisfy the ">= 0.0.0" condition. I'm not completely sure about this, so it's worth double-checking.
@olafurpg, The referenced PR only removed the feature flag that gated the Cody context filters usage before client support was in place. Without this flag enabled, admins would receive an error when trying to define Your PR is necessary to allow support for clients other than |
Previously, we send an appropriate Cody agent client name "eclipse". This was problematic because it meant that the plugin didn't work with Enterprise server instances that have enabled context filters due to a check against old versions of clients that may not support context filters. The server checks have been fixed in https://github.com/sourcegraph/sourcegraph/pull/63855 but we need to wait a few months before being able to send "Eclipse" as a client name when Enterprise users have completed upgrading to a newer Sourcegraph version. The big downside with this solution is that it means all the telemetry in the plugin indicates that events are coming from JetBrains. We can fix this problem by extending the Cody agent server protocol to accept a dedicated setting to override only the HTTP client name for LLM completion requests. This work is tracked in the issue CODY-2978.
Fixes CODY-2888
Previously, Sourcegraph Enterprise instances with context filters enabled rejected requests from all unknown clients out of concern that they might not respect context filters. This behavior makes it incredibly impractical to release now agent-based clients (CLI, Eclipse, Visual Studio, Neovim, ..) that do respect context filters out of the box thanks to the reused logic in the Cody agent.
This logic suffers from both false positives and false negatives:
Now, with this change, Sourcegraph Enterprise instances only reject requests from old versions of Cody clients that are known to not support context filters. This ensures we never have false positives or false negatives.
Test plan
See updated test case which accepts a request from an unknown "sublime" client.
Changelog