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

fix: upgrade @copilot-extensions/preview-sdk to v2 #5

Merged
merged 8 commits into from
Sep 6, 2024
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"license": "ISC",
"description": "",
"dependencies": {
"@copilot-extensions/preview-sdk": "^1.0.0",
"@copilot-extensions/preview-sdk": "^2.6.1",
"express": "^4.19.2",
"openai": "^4.55.0"
},
Expand All @@ -22,4 +22,4 @@
"tsx": "^4.18.0",
"typescript": "^5.5.4"
}
}
}
103 changes: 77 additions & 26 deletions src/index.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,59 @@
import express from "express";
import { createServer, IncomingMessage } from "node:http";

import { verifyAndParseRequest, createAckEvent } from "@copilot-extensions/preview-sdk";
import OpenAI from "openai";
import { verifySignatureMiddleware } from "./validate-signature.js";

import { describeModel } from "./functions/describe-model.js";
import { executeModel } from "./functions/execute-model.js";
import { listModels } from "./functions/list-models.js";
import { RunnerResponse } from "./functions.js";
import { recommendModel } from "./functions/recommend-model.js";
import { ModelsAPI } from "./models-api.js";
const app = express();

app.post("/", verifySignatureMiddleware, express.json(), async (req, res) => {
const server = createServer(async (request, response) => {
if (request.method === "GET") {
response.statusCode = 200;
response.end(`OK`);
return;
}

const body = await getBody(request);

let verifyAndParseRequestResult: Awaited<ReturnType<typeof verifyAndParseRequest>>;
let apiKey: string;
try {
const signature = request.headers["github-public-key-signature"] as string;
const keyID = request.headers["github-public-key-identifier"] as string;
apiKey = request.headers["x-github-token"] as string;
verifyAndParseRequestResult = await verifyAndParseRequest(body, signature, keyID, {
token: apiKey,
});
} catch (err) {
console.error(err);
response.statusCode = 401
response.end("Unauthorized");
return
}

const { isValidRequest, payload } = verifyAndParseRequestResult

if (!isValidRequest) {
console.log("Signature verification failed");
response.statusCode = 401
response.end("Unauthorized");
}

console.log("Signature verified");

// Use the GitHub API token sent in the request
const apiKey = req.get("X-GitHub-Token");
if (!apiKey) {
res.status(400).end();
response.statusCode = 400
response.end()
return;
}

response.write(createAckEvent().toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't love the idea of this method - it feels unnecessary, and "Ack" isn't a super common term. The functionality of the integration doesn't change at all without calling this.

Separately: can we make the create*Event methods just return a string? Is there a case where you need the object returned, and not just the string?

Copy link
Collaborator Author

@gr2m gr2m Sep 5, 2024

Choose a reason for hiding this comment

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

The functionality of the integration doesn't change at all without calling this.

it does put the chat UI into "the agent received your request and is responding" mode, the UI changes. I would consider it best practice. But we can remove it if you prefer

Separately: can we make the create*Event methods just return a string? Is there a case where you need the object returned, and not just the string?

These are super low-level methods. I was hoping that response.write would call the .toString() method directly, but alas, it doesn't. I think it makes sense for them to create an object in case you want to use your own logic to transport them. Once they are a string, the structural data can no longer be retrieved.

The higher-level API would make the necessity of calling .toString() go away anyway though:
https://github.com/copilot-extensions/preview-sdk.js/blob/9533a1fd20409332c8f1cf60ac15e2421347ce55/dreamcode.md#L35-L43

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you know what I changed my mind, let's just have them generate strings, it's a problem when it's a problem. I'll apply the change in #6 if that's okay with you, let's get that one merged first, ok?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

and "Ack" isn't a super common term

yeah naming is hard 🤔 What would you call it? Maybe avoid the abbreviation createAcknowledgeEvent? Or call it createConfirmRequestEvent()? The latter is too close to createConfirmationEvent() for my taste though

Choose a reason for hiding this comment

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

Not opposed to this returning strings. I don't have a good stance on if this should be an object, unless there's a primary adoption for something consuming this that would need a certain shape. Folks will be passing in objects to these methods, in most cases, so they should have a handle to the non-string version if needed, before these methods are called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll go with createAcknowledgementEvent() for now, more coherent than createAcknowledgeEvent() with the other create*Event() methods

Copy link
Collaborator

Choose a reason for hiding this comment

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

it does put the chat UI into "the agent received your request and is responding" mode, the UI changes. I would consider it best practice. But we can remove it if you prefer

This points to an improvement we should make to the platform; we already know when we've started communicating to an external agent. I'd prefer to remove that for now if that's cool!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed it via 7d5dd21


// List of functions that are available to be called
const modelsAPI = new ModelsAPI(apiKey);
const functions = [listModels, describeModel, executeModel, recommendModel];
Expand Down Expand Up @@ -50,13 +87,14 @@ app.post("/", verifySignatureMiddleware, express.json(), async (req, res) => {
"<-- END OF LIST OF MODELS -->",
].join("\n"),
},
...req.body.messages,
].concat(req.body.messages);
...payload.messages,
].concat(payload.messages);

console.time("tool-call");
const toolCaller = await capiClient.chat.completions.create({
stream: false,
model: "gpt-4",
// @ts-expect-error - TODO @gr2m - type incompatibility between @openai/api and @copilot-extensions/preview-sdk
messages: toolCallMessages,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? Why wouldn't we make the preview-sdk's types compatible?

Moreover: I'm not sure I agree that the SDK should contain an SDK for OpenAI. Like in #6, we're also calling the GitHub Models API - that's an OpenAI-compatible API, and has nothing to do with Copilot. I wouldn't expect or want to use a Copilot SDK for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the prompt() method originated from discussions in GitHub Models, where we wanted to simplify the getting started documentation by using our own SDK that removes some of the complexities. However that related to Azure's SDKs.

I can bring back the OpenAI SDK if you prefer, it's just quite a heavy dependency with a lot of functionality you don't need here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I removed the @ts-expect-error in 7507a09. If anything it belongs into #6.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's just quite a heavy dependency with a lot of functionality you don't need here

I don't really agree - and it's more a question of what makes most sense for the models extension vs. all Copilot Extensions. As an Extension that calls multiple models, the more robust and well-used SDK feels more appropriate. Besides which, for an example repository I'd bias more towards clarity and adherence to the ecosystem over performance or dependency weight.

tool_choice: "auto",
tools: functions.map((f) => f.tool),
Expand All @@ -74,15 +112,16 @@ app.post("/", verifySignatureMiddleware, express.json(), async (req, res) => {
const stream = await capiClient.chat.completions.create({
stream: true,
model: "gpt-4",
messages: req.body.messages,
// @ts-expect-error - TODO @gr2m - type incompatibility between @openai/api and @copilot-extensions/preview-sdk
messages: payload.messages,
});

for await (const chunk of stream) {
const chunkStr = "data: " + JSON.stringify(chunk) + "\n\n";
res.write(chunkStr);
response.write(chunkStr);
}
res.write("data: [DONE]\n\n");
res.end();
response.write("data: [DONE]\n\n");
response.end();
return;
}

Expand All @@ -102,10 +141,12 @@ app.post("/", verifySignatureMiddleware, express.json(), async (req, res) => {

console.log("\t with args", args);
const func = new funcClass(modelsAPI);
functionCallRes = await func.execute(req.body.messages, args);
// @ts-expect-error - TODO @gr2m - type incompatibility between @openai/api and @copilot-extensions/preview-sdk
functionCallRes = await func.execute(payload.messages, args);
} catch (err) {
console.error(err);
res.status(500).end();
response.statusCode = 500
response.end();
return;
}
console.timeEnd("function-exec");
Expand All @@ -123,23 +164,33 @@ app.post("/", verifySignatureMiddleware, express.json(), async (req, res) => {
console.time("streaming");
for await (const chunk of stream) {
const chunkStr = "data: " + JSON.stringify(chunk) + "\n\n";
res.write(chunkStr);
response.write(chunkStr);
}
res.write("data: [DONE]\n\n");
response.write("data: [DONE]\n\n");
console.timeEnd("streaming");
res.end();
response.end();
} catch (err) {
console.error(err);
res.status(500).end();
response.statusCode = 500
response.end()
}
});

// Health check
app.get("/", (req, res) => {
res.send("OK");
});
const port = process.env.PORT || "3000"
server.listen(port);
console.log(`Server running at http://localhost:${port}`);

const port = Number(process.env.PORT || "3000");
app.listen(port, () => {
console.log(`Server is running on http://localhost:${port}`);
});
function getBody(request: IncomingMessage): Promise<string> {
return new Promise((resolve) => {
const bodyParts: any[] = [];
let body;
request
.on("data", (chunk) => {
bodyParts.push(chunk);
})
.on("end", () => {
body = Buffer.concat(bodyParts).toString();
resolve(body);
});
});
}
31 changes: 0 additions & 31 deletions src/validate-signature.ts

This file was deleted.