Allow Github Copilot to be used with Terminal Chat #18014

PankajBhojwani · 2024-10-08T18:50:38Z

Summary of the Pull Request

Implements GithubCopilotLLMProvider, which is an implementation of ILMProvider that leverages Github Copilot
Github auth flow can be initiated from the settings UI
Modifies the ILMProvider interface to include an IBrandingData interface, that allows a provider to specify how it wants certain elements of the TerminalChat UI to look
Modified the various telemetry events to include the name of the currently connected provider

Validation Steps Performed

Auth flow works
Automatic refresh of the auth tokens works, meaning you don't need to repeat the auth flow every few days

PR Checklist

Closes #xxx
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

DHowett

i'm at 43/48 files (sorry for the delay!)

src/cascadia/QueryExtension/AzureLLMProvider.h

src/cascadia/QueryExtension/ExtensionPalette.h

DHowett · 2024-10-23T22:42:43Z

src/cascadia/QueryExtension/ExtensionPalette.xaml

+                            HorizontalAlignment="Left"
+                            VerticalAlignment="Bottom"
+                            Spacing="4">
+                    <mux:ImageIcon Source="{x:Bind BadgeBitmapImage}"/>


Highly amazed this worked. It should have failed because WUX and MUX image sources are different (????)

I wish I had an answer for this haha

src/cascadia/QueryExtension/ExtensionPalette.xaml

src/cascadia/TerminalSettingsEditor/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/TerminalPage.cpp

DHowett · 2024-10-23T23:30:39Z

src/cascadia/QueryExtension/GithubCopilotLLMProvider.cpp

+            const auto randomStateString = unbox_value_or<hstring>(authValues.TryLookup(stateKey).try_as<IPropertyValue>(), L"");
+            // only handle this if the state strings match
+            if (randomStateString == parsedUrl.QueryParsed().GetFirstValueByName(stateKey))


do we also confirm that the random state string matches the one stored on the app?

Not sure I understand the question - randomStateString here is the one stored on the app

src/cascadia/QueryExtension/AzureLLMProvider.h

src/cascadia/QueryExtension/ExtensionPalette.cpp

src/cascadia/QueryExtension/GithubCopilotLLMProvider.cpp

…nto dev/pabhoj/featurellm_openai

DHowett · 2024-10-28T20:57:26Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                        authValuesJson.SetNamedValue(L"url", WDJ::JsonValue::CreateStringValue(uriString));
+                        authValuesJson.SetNamedValue(L"state", WDJ::JsonValue::CreateStringValue(randomStateString));
+
+                        _createAndSetAuthenticationForLMProvider(LLMProvider::GithubCopilot, authValuesJson.ToString());


this is a JSON key contract between App and Provider that we should have been avoiding - what does it communicate, and why does it need to know the internal format of the auth blob?

This is not the blob that ends up getting stored, this is the one to complete the auth flow, i.e. to hand the correct uri (which contains the necessary code to complete the authentication) and the random state string to the provider. The one that eventually gets stored has a format unknown to the App

Eventually, this should be promoted to being a real function on an interface like IOAuthAuthenticatedProvider or similar. We don't need to do that now, but it IS still currently effectively a secret and undocumented contract (and the kind of thing I hate: using JSON to pass data from one part of the app to another (which is different from the 'opaque blob' thing, which uses JSON as an implementation detail for one component to save data about ITSELF))

src/cascadia/QueryExtension/ExtensionPalette.cpp

src/cascadia/TerminalSettingsModel/AIConfig.cpp

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

DHowett

FWIW, you don't have to change the Azure and OAI ones to use the opaque blob thing. It's fine - the app already HAS to peek inside it.

src/cascadia/QueryExtension/AzureLLMProvider.cpp

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

DHowett · 2024-10-28T21:10:59Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+            const auto uriString{ uriArgs.Uri() };
+            if (!uriString.empty())
+            {
+                Windows::Foundation::Uri uri{ uriString };


technically this will throw if the URI string is wrong. wt handle-uri foobarbaz will maybe make terminal blow up

You may want to fix this in a followup!

DHowett · 2024-10-28T21:13:03Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                        authValuesJson.SetNamedValue(L"url", WDJ::JsonValue::CreateStringValue(uriString));
+                        authValuesJson.SetNamedValue(L"state", WDJ::JsonValue::CreateStringValue(randomStateString));
+
+                        _createAndSetAuthenticationForLMProvider(LLMProvider::GithubCopilot, authValuesJson.ToString());


Eventually, this should be promoted to being a real function on an interface like IOAuthAuthenticatedProvider or similar. We don't need to do that now, but it IS still currently effectively a secret and undocumented contract (and the kind of thing I hate: using JSON to pass data from one part of the app to another (which is different from the 'opaque blob' thing, which uses JSON as an implementation detail for one component to save data about ITSELF))

src/cascadia/QueryExtension/GithubCopilotLLMProvider.cpp

PankajBhojwani added 30 commits June 6, 2024 14:40

got an idl

b44216b

works

e78e4d0

allow

ac83d76

conflict

4b944e9

use id here too

ef406ee

have terminal page initialize the llmprovider

4fb4ca4

format

e5afbae

consts

32b3d68

works on palette side

1700a92

open ai in settings, llmprovider enum

08dd951

aiconfig struct

1934b30

move more things to ai info

96a489a

works on palette side

6052a8d

open ai in settings, llmprovider enum

4add2c9

aiconfig struct

c3885ff

move more things to ai info

646c28b

github copilot impl works

fb7ff3c

handle uri action

8bf23bc

rough edges, but roundtrip works

65c2587

mark a couple todos

73eb2af

active provider buttons

8e560e2

rename to lmprovider

f9e9326

spelling conflict

4133239

conflicts

e545ca2

spell

dfad8d9

array of accepted models

5139b88

cleanup this comment

dfbdc75

placeholder, remove device flow code

2819606

refresh tokens works

19e73db

error dialog for failed auth

82125f9

PankajBhojwani added 4 commits October 22, 2024 17:38

nits

b2e38af

zwstring_view

ce4a1de

conflict

95e721c

store the last response to bubble in case of errors

596d17e

DHowett reviewed Oct 23, 2024

View reviewed changes

PankajBhojwani added 2 commits October 24, 2024 11:41

rename to attribution, getters only

2fc21ef

branding too

bcfc1d4

DHowett reviewed Oct 24, 2024

View reviewed changes

src/cascadia/QueryExtension/AzureLLMProvider.h Outdated Show resolved Hide resolved

some fixes

2bb6d7f

lhecker reviewed Oct 24, 2024

View reviewed changes

src/cascadia/QueryExtension/ExtensionPalette.cpp Show resolved Hide resolved

src/cascadia/QueryExtension/GithubCopilotLLMProvider.cpp Outdated Show resolved Hide resolved

src/cascadia/QueryExtension/GithubCopilotLLMProvider.cpp Outdated Show resolved Hide resolved

PankajBhojwani added 8 commits October 24, 2024 16:04

auth values as string

6a694ae

use winrt::hstring

3ffaafc

wait these didn't save

d0847a1

strong

7234b9d

safer coroutines

b969baf

turn on feature for new build

e0d7172

iasyncaction instead

bd34e90

use a grid because stackpanel is the worst

df9a233

Base automatically changed from dev/pabhoj/featurellm_openai to feature/llm October 28, 2024 19:34

PankajBhojwani added 2 commits October 28, 2024 12:46

Merge branch 'feature/llm' of https://github.com/microsoft/terminal i…

a6deddf

…nto dev/pabhoj/featurellm_openai

conflict

0564bc5

PankajBhojwani force-pushed the dev/pabhoj/featurellm_capi branch from df03bf9 to 0564bc5 Compare October 28, 2024 20:02

DHowett reviewed Oct 28, 2024

View reviewed changes

src/cascadia/QueryExtension/ExtensionPalette.cpp Show resolved Hide resolved

DHowett reviewed Oct 28, 2024

View reviewed changes

src/cascadia/TerminalSettingsModel/AIConfig.cpp Show resolved Hide resolved

DHowett reviewed Oct 28, 2024

View reviewed changes

src/cascadia/TerminalApp/Resources/en-US/Resources.resw Show resolved Hide resolved

unpackaged check

9a30666

DHowett approved these changes Oct 28, 2024

View reviewed changes

DHowett merged commit b2524f9 into feature/llm Oct 28, 2024
20 checks passed

DHowett deleted the dev/pabhoj/featurellm_capi branch October 28, 2024 23:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Github Copilot to be used with Terminal Chat #18014

Allow Github Copilot to be used with Terminal Chat #18014

PankajBhojwani commented Oct 8, 2024

DHowett left a comment

DHowett Oct 23, 2024

PankajBhojwani Oct 24, 2024

DHowett Oct 23, 2024

PankajBhojwani Oct 24, 2024 •

edited

Loading

DHowett Oct 28, 2024

PankajBhojwani Oct 28, 2024

DHowett Oct 28, 2024

DHowett left a comment

DHowett Oct 28, 2024

DHowett Oct 28, 2024

DHowett Oct 28, 2024

Allow Github Copilot to be used with Terminal Chat #18014

Allow Github Copilot to be used with Terminal Chat #18014

Conversation

PankajBhojwani commented Oct 8, 2024

Summary of the Pull Request

Validation Steps Performed

PR Checklist

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani Oct 24, 2024 •

edited

Loading