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

Send client-name, client-version query string parameters to completions #4018

Merged
merged 3 commits into from
May 7, 2024

Conversation

dominiccooney
Copy link
Contributor

@dominiccooney dominiccooney commented May 2, 2024

Closes CODY-1300

Sourcegraph 5.4 or later completions endpoint will start to check a minimum version asserted by the client. This starts sending the relevant header.

Part of sourcegraph/sourcegraph#62116

Test plan

CI

Note updated recordings have client-name, client-version query string parameters.

Manual test

Sign in to sg02.sourcegraphcloud.com and do a chat. You should see an error message like "Request Failed: ClientCodyIgnoreCompatibilityError: Cody for vscode version "1.16.0" doesn't match version constraint ">= 1.20.0" Note, the "vscode" and "1.16.0" strings come from the client.

Screenshot 2024-05-02 at 19 26 28

Generate an autocomplete with alt-\ and check the Cody by Sourcegraph output category contains a cody.completion/error log:

    "privateMetadata": {
      "message": "Request to https://sg02.sourcegraphcloud.com/.api/completions/code?client-name=vscode&client-version=1.16.0 failed with 406 Not Acceptable: ClientCodyIgnoreCompatibilityError: Cody for vscode version \"1.16.0\" doesn't match version constraint \">= 1.20.0\"\n"
    }

JetBrains: Run JetBrains plugin with this agent and do similar actions. Error messages indicate the right client name and version are sent:

Screenshot 2024-05-02 at 19 38 58

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.

Nice! But this misses lib/shared/src/sourcegraph-api/completions/browserClient.ts let's make sure to add the headers there as well even though it's unclear wether we actually have customers using it 🤔

lib/shared/src/sourcegraph-api/client-name-version.ts Outdated Show resolved Hide resolved
vscode/src/local-context/symf.test.ts Outdated Show resolved Hide resolved
@dominiccooney dominiccooney force-pushed the dpc/client-name-version branch from 4f79ff1 to e02ebc4 Compare May 2, 2024 13:25
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.

Thank you 🙇

Copy link
Member

@valerybugakov valerybugakov left a comment

Choose a reason for hiding this comment

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

👀

biome

Update agent recordings

Add client-name-version

Revert spurious graph-context.test.ts change.

Add missing symf recordings, update graph snapshot.

De-flake an e2e test.

Lowercase the client name, Sourcegraph server expects lowercase strings.

Update agent recordings.

Feedback
@dominiccooney dominiccooney force-pushed the dpc/client-name-version branch from e02ebc4 to ce5aec2 Compare May 6, 2024 23:28
@dominiccooney dominiccooney merged commit 6c65981 into main May 7, 2024
18 of 19 checks passed
@dominiccooney dominiccooney deleted the dpc/client-name-version branch May 7, 2024 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants