Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a comprehensive getting started tutorial for the GitHub Copilot SDK with examples in TypeScript, Python, Go, and .NET, along with a memory leak fix in the Node.js session implementation.
Changes:
- Fixed a memory leak in the Node.js SDK by properly clearing timeout handlers after Promise.race() completes
- Added an 884-line tutorial document covering basic usage, streaming, custom tools, and interactive assistants across all four supported languages
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| nodejs/src/session.ts | Fixed memory leak by storing and clearing timeout ID in sendAndWait method |
| docs/getting-started.md | New comprehensive tutorial with multi-language examples for getting started with the Copilot SDK |
Comments suppressed due to low confidence (2)
docs/getting-started.md:734
- The Python define_tool usage in the interactive assistant example has the same issues as the earlier example. The tool handler needs to use a Pydantic BaseModel for parameters and include the invocation parameter in the function signature.
@define_tool(description="Get the current weather for a city")
async def get_weather(params: dict) -> dict:
city = params["city"]
conditions = ["sunny", "cloudy", "rainy", "partly cloudy"]
temp = random.randint(50, 80)
condition = random.choice(conditions)
return {"city": city, "temperature": f"{temp}°F", "condition": condition}
docs/getting-started.md:460
- Missing required imports for the Python tool definition. The code needs to import Pydantic's BaseModel and Field to properly define tool parameters, and ToolInvocation from copilot. Add:
from pydantic import BaseModel, Field
from copilot import ToolInvocation
import asyncio
import random
import sys
from copilot import CopilotClient
from copilot.tools import define_tool
from copilot.generated.session_events import SessionEventType
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ```python | ||
| import asyncio | ||
| import random | ||
| import sys | ||
| from copilot import CopilotClient | ||
| from copilot.tools import define_tool | ||
| from copilot.generated.session_events import SessionEventType |
There was a problem hiding this comment.
Missing required imports for Pydantic BaseModel, Field, and ToolInvocation in the interactive assistant example.
This issue also appears in the following locations of the same file:
- line 455
| @define_tool(description="Get the current weather for a city") | ||
| async def get_weather(params: dict) -> dict: | ||
| city = params["city"] | ||
| # In a real app, you'd call a weather API here | ||
| conditions = ["sunny", "cloudy", "rainy", "partly cloudy"] | ||
| temp = random.randint(50, 80) | ||
| condition = random.choice(conditions) | ||
| return {"city": city, "temperature": f"{temp}°F", "condition": condition} |
There was a problem hiding this comment.
The Python define_tool usage is incorrect. According to the Python SDK documentation and tests, define_tool requires:
- A Pydantic BaseModel class to define the parameter schema (not a plain dict)
- The handler function signature should include both params and invocation parameters:
def get_weather(params: WeatherParams, invocation: ToolInvocation)
The function should define a WeatherParams class like:
class WeatherParams(BaseModel):
city: str = Field(description="The city name")
Without this, Copilot won't know what parameters the tool expects and cannot properly call it.
This issue also appears in the following locations of the same file:
- line 728
cb0ec96 to
9b7cb96
Compare
steveward
left a comment
There was a problem hiding this comment.
Looks great from a docs perspective! ✨
| } | ||
| }); | ||
|
|
||
| await session.sendAndWait({ prompt: "Tell me a short joke" }); |
There was a problem hiding this comment.
I think it would be nice to have an example that uses send not just sendAndWait. Maybe something that prompts the user to input a message and then sends it?
| @@ -0,0 +1,884 @@ | |||
| # Build Your First Copilot-Powered App | |||
There was a problem hiding this comment.
would it be worth splitting having two tutorials one for python/one for node?
There was a problem hiding this comment.
@ashleywolf - thoughts here? Right now everything is in one tutorial and you get to pick your language. But we could punt out to separate files for just Node/Python instead of trying to show everything. Would be cleaner but we lose the Go/.NET.
Getting started tutorial with examples in TypeScript, Python, Go and .NET