From e6f41736dd4b669da98f2df4e6ac12e1aebe288f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 05:22:57 +0000 Subject: [PATCH 1/7] Initial plan From 59fad69287a76818d139a20a88559228af9b1c8e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 05:47:29 +0000 Subject: [PATCH 2/7] Update interface and all calls to support HTTP logging filename parameter Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- cmd/eval/eval.go | 8 +++++- cmd/eval/eval_test.go | 14 +++++------ cmd/generate/generate.go | 6 +++++ cmd/generate/generate_test.go | 10 ++++---- cmd/generate/llm.go | 2 +- cmd/generate/pipeline.go | 10 ++++---- cmd/run/run.go | 15 ++++++----- cmd/run/run_test.go | 10 ++++---- internal/azuremodels/azure_client.go | 25 +++++++++---------- internal/azuremodels/azure_client_test.go | 6 ++--- internal/azuremodels/client.go | 3 ++- internal/azuremodels/mock_client.go | 8 +++--- .../azuremodels/unauthenticated_client.go | 2 +- 13 files changed, 67 insertions(+), 52 deletions(-) diff --git a/cmd/eval/eval.go b/cmd/eval/eval.go index 566bd0df..209b15a6 100644 --- a/cmd/eval/eval.go +++ b/cmd/eval/eval.go @@ -111,6 +111,9 @@ func NewEvalCommand(cfg *command.Config) *cobra.Command { // Get the org flag org, _ := cmd.Flags().GetString("org") + // Get the http-log flag + httpLog, _ := cmd.Flags().GetString("http-log") + // Load the evaluation prompt file evalFile, err := loadEvaluationPromptFile(promptFilePath) if err != nil { @@ -124,6 +127,7 @@ func NewEvalCommand(cfg *command.Config) *cobra.Command { evalFile: evalFile, jsonOutput: jsonOutput, org: org, + httpLog: httpLog, } err = handler.runEvaluation(cmd.Context()) @@ -139,6 +143,7 @@ func NewEvalCommand(cfg *command.Config) *cobra.Command { cmd.Flags().Bool("json", false, "Output results in JSON format") cmd.Flags().String("org", "", "Organization to attribute usage to (omitting will attribute usage to the current actor") + cmd.Flags().String("http-log", "", "Path to log HTTP requests to (optional)") return cmd } @@ -148,6 +153,7 @@ type evalCommandHandler struct { evalFile *prompt.File jsonOutput bool org string + httpLog string } func loadEvaluationPromptFile(filePath string) (*prompt.File, error) { @@ -372,7 +378,7 @@ func (h *evalCommandHandler) callModelWithRetry(ctx context.Context, req azuremo const maxRetries = 3 for attempt := 0; attempt <= maxRetries; attempt++ { - resp, err := h.client.GetChatCompletionStream(ctx, req, h.org) + resp, err := h.client.GetChatCompletionStream(ctx, req, h.org, h.httpLog) if err != nil { var rateLimitErr *azuremodels.RateLimitError if errors.As(err, &rateLimitErr) { diff --git a/cmd/eval/eval_test.go b/cmd/eval/eval_test.go index 59fc128f..cecb3937 100644 --- a/cmd/eval/eval_test.go +++ b/cmd/eval/eval_test.go @@ -162,7 +162,7 @@ evaluators: cfg := command.NewConfig(out, out, client, true, 100) // Mock a response that returns "4" for the LLM evaluator - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { Choices: []azuremodels.ChatChoice{ @@ -228,7 +228,7 @@ evaluators: client := azuremodels.NewMockClient() // Mock a simple response - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { // Create a mock reader that returns "test response" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -284,7 +284,7 @@ evaluators: client := azuremodels.NewMockClient() // Mock a response that will fail the evaluator - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { Choices: []azuremodels.ChatChoice{ @@ -347,7 +347,7 @@ evaluators: // Mock responses for both test cases callCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { callCount++ var response string if callCount == 1 { @@ -445,7 +445,7 @@ evaluators: require.NoError(t, err) client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { response := "hello world" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -528,7 +528,7 @@ evaluators: require.NoError(t, err) client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { response := "hello world" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -590,7 +590,7 @@ evaluators: client := azuremodels.NewMockClient() var capturedRequest azuremodels.ChatCompletionOptions - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { capturedRequest = req response := `{"message": "hello world", "confidence": 0.95}` reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ diff --git a/cmd/generate/generate.go b/cmd/generate/generate.go index 483f66fd..607d47fd 100644 --- a/cmd/generate/generate.go +++ b/cmd/generate/generate.go @@ -17,6 +17,7 @@ type generateCommandHandler struct { client azuremodels.Client options *PromptPexOptions org string + httpLog string } // NewGenerateCommand returns a new command to generate tests using PromptPex. @@ -50,6 +51,9 @@ func NewGenerateCommand(cfg *command.Config) *cobra.Command { // Get organization org, _ := cmd.Flags().GetString("org") + // Get http-log flag + httpLog, _ := cmd.Flags().GetString("http-log") + // Create the command handler handler := &generateCommandHandler{ ctx: cmd.Context(), @@ -57,6 +61,7 @@ func NewGenerateCommand(cfg *command.Config) *cobra.Command { client: cfg.Client, options: options, org: org, + httpLog: httpLog, } // Create PromptPex context @@ -97,6 +102,7 @@ func AddCommandLineFlags(cmd *cobra.Command) { flags.String("custom-metric", "", "Custom evaluation metric") flags.Float64("temperature", 0.0, "Temperature for model inference") flags.Bool("verbose", false, "Enable verbose output including LLM payloads") + flags.String("http-log", "", "Path to log HTTP requests to (optional)") } // parseFlags parses command-line flags and applies them to the options diff --git a/cmd/generate/generate_test.go b/cmd/generate/generate_test.go index c429b73c..4e37d9c4 100644 --- a/cmd/generate/generate_test.go +++ b/cmd/generate/generate_test.go @@ -207,7 +207,7 @@ messages: // Setup mock client to return error client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { return nil, errors.New("Mock API error") } @@ -241,7 +241,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() callCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { callCount++ var response string @@ -314,7 +314,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content @@ -382,7 +382,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content @@ -451,7 +451,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content diff --git a/cmd/generate/llm.go b/cmd/generate/llm.go index c4411e82..bda9de9e 100644 --- a/cmd/generate/llm.go +++ b/cmd/generate/llm.go @@ -24,7 +24,7 @@ func (h *generateCommandHandler) callModelWithRetry(step string, req azuremodels //nolint:gocritic,revive // TODO defer sp.Stop() - resp, err := h.client.GetChatCompletionStream(ctx, req, h.org) + resp, err := h.client.GetChatCompletionStream(ctx, req, h.org, h.httpLog) if err != nil { var rateLimitErr *azuremodels.RateLimitError if errors.As(err, &rateLimitErr) { diff --git a/cmd/generate/pipeline.go b/cmd/generate/pipeline.go index a06a375a..3a4ca983 100644 --- a/cmd/generate/pipeline.go +++ b/cmd/generate/pipeline.go @@ -388,7 +388,7 @@ func (h *generateCommandHandler) runSingleTestWithContext(input, modelName strin Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) if err != nil { return "", err } @@ -469,7 +469,7 @@ Compliance:`, rules, output) Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) if err != nil { return EvalResultUnknown, err @@ -510,7 +510,7 @@ Score (0-1):`, metric, output) Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) if err != nil { return 0.0, err @@ -615,7 +615,7 @@ Generate variations in JSON format as an array of objects with "scenario", "test Temperature: util.Ptr(0.5), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) if err != nil { return nil, err @@ -676,7 +676,7 @@ Analysis:`, strings.Join(testSummary, "\n")) Temperature: util.Ptr(0.2), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) if err != nil { return err diff --git a/cmd/run/run.go b/cmd/run/run.go index d0f58991..ce7fddc4 100644 --- a/cmd/run/run.go +++ b/cmd/run/run.go @@ -423,6 +423,7 @@ func NewRunCommand(cfg *command.Config) *cobra.Command { cmd.Flags().String("top-p", "", "Controls text diversity by selecting the most probable words until a set probability is reached.") cmd.Flags().String("system-prompt", "", "Prompt the system.") cmd.Flags().String("org", "", "Organization to attribute usage to (omitting will attribute usage to the current actor") + cmd.Flags().String("http-log", "", "Path to log HTTP requests to (optional)") return cmd } @@ -465,14 +466,16 @@ func parseTemplateVariables(flags *pflag.FlagSet) (map[string]string, error) { } type runCommandHandler struct { - ctx context.Context - cfg *command.Config - client azuremodels.Client - args []string + ctx context.Context + cfg *command.Config + client azuremodels.Client + args []string + httpLog string } func newRunCommandHandler(cmd *cobra.Command, cfg *command.Config, args []string) *runCommandHandler { - return &runCommandHandler{ctx: cmd.Context(), cfg: cfg, client: cfg.Client, args: args} + httpLog, _ := cmd.Flags().GetString("http-log") + return &runCommandHandler{ctx: cmd.Context(), cfg: cfg, client: cfg.Client, args: args, httpLog: httpLog} } func (h *runCommandHandler) loadModels() ([]*azuremodels.ModelSummary, error) { @@ -551,7 +554,7 @@ func validateModelName(modelName string, models []*azuremodels.ModelSummary) (st } func (h *runCommandHandler) getChatCompletionStreamReader(req azuremodels.ChatCompletionOptions, org string) (sse.Reader[azuremodels.ChatCompletion], error) { - resp, err := h.client.GetChatCompletionStream(h.ctx, req, org) + resp, err := h.client.GetChatCompletionStream(h.ctx, req, org, h.httpLog) if err != nil { return nil, err } diff --git a/cmd/run/run_test.go b/cmd/run/run_test.go index 94db2b63..e9919950 100644 --- a/cmd/run/run_test.go +++ b/cmd/run/run_test.go @@ -44,7 +44,7 @@ func TestRun(t *testing.T) { Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), } getChatCompletionCallCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { getChatCompletionCallCount++ return chatResp, nil } @@ -122,7 +122,7 @@ messages: }, }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -189,7 +189,7 @@ messages: }, }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -281,7 +281,7 @@ messages: }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -367,7 +367,7 @@ messages: } var capturedRequest azuremodels.ChatCompletionOptions - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { capturedRequest = req reply := "hello this is a test response" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ diff --git a/internal/azuremodels/azure_client.go b/internal/azuremodels/azure_client.go index baafcb42..0cd9a93a 100644 --- a/internal/azuremodels/azure_client.go +++ b/internal/azuremodels/azure_client.go @@ -45,7 +45,7 @@ func NewAzureClient(httpClient *http.Client, authToken string, cfg *AzureClientC } // GetChatCompletionStream returns a stream of chat completions using the given options. -func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { +func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { // Check for o1 models, which don't support streaming if req.Model == "o1-mini" || req.Model == "o1-preview" || req.Model == "o1" { req.Stream = false @@ -67,19 +67,18 @@ func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompl inferenceURL = c.cfg.InferenceRoot + "/" + c.cfg.InferencePath } - // TODO: remove logging - // Write request details to llm.http file for debugging - if os.Getenv("DEBUG") != "" { - httpFile, err := os.OpenFile("llm.http", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + // Write request details to specified log file for debugging + if httpLogFile != "" { + logFile, err := os.OpenFile(httpLogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err == nil { - defer httpFile.Close() - fmt.Fprintf(httpFile, "### %s\n", time.Now().Format(time.RFC3339)) - fmt.Fprintf(httpFile, "POST %s\n", inferenceURL) - fmt.Fprintf(httpFile, "Authorization: Bearer {{$processEnv GITHUB_TOKEN}}\n") - fmt.Fprintf(httpFile, "Content-Type: application/json\n") - fmt.Fprintf(httpFile, "x-ms-useragent: github-cli-models\n") - fmt.Fprintf(httpFile, "x-ms-user-agent: github-cli-models\n") - fmt.Fprintf(httpFile, "\n%s\n\n", string(bodyBytes)) + defer logFile.Close() + fmt.Fprintf(logFile, "### %s\n", time.Now().Format(time.RFC3339)) + fmt.Fprintf(logFile, "POST %s\n", inferenceURL) + fmt.Fprintf(logFile, "Authorization: Bearer {{$processEnv GITHUB_TOKEN}}\n") + fmt.Fprintf(logFile, "Content-Type: application/json\n") + fmt.Fprintf(logFile, "x-ms-useragent: github-cli-models\n") + fmt.Fprintf(logFile, "x-ms-user-agent: github-cli-models\n") + fmt.Fprintf(logFile, "\n%s\n\n", string(bodyBytes)) } } diff --git a/internal/azuremodels/azure_client_test.go b/internal/azuremodels/azure_client_test.go index a8b6bf23..132d11e6 100644 --- a/internal/azuremodels/azure_client_test.go +++ b/internal/azuremodels/azure_client_test.go @@ -63,7 +63,7 @@ func TestAzureClient(t *testing.T) { }, } - chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "") + chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "", "") require.NoError(t, err) require.NotNil(t, chatCompletionStreamResp) @@ -139,7 +139,7 @@ func TestAzureClient(t *testing.T) { }, } - chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "") + chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "", "") require.NoError(t, err) require.NotNil(t, chatCompletionStreamResp) @@ -181,7 +181,7 @@ func TestAzureClient(t *testing.T) { Messages: []ChatMessage{{Role: "user", Content: util.Ptr("Tell me a story, test model.")}}, } - chatCompletionResp, err := client.GetChatCompletionStream(ctx, opts, "") + chatCompletionResp, err := client.GetChatCompletionStream(ctx, opts, "", "") require.Error(t, err) require.Nil(t, chatCompletionResp) diff --git a/internal/azuremodels/client.go b/internal/azuremodels/client.go index a3f68ca3..053104b5 100644 --- a/internal/azuremodels/client.go +++ b/internal/azuremodels/client.go @@ -5,7 +5,8 @@ import "context" // Client represents a client for interacting with an API about models. type Client interface { // GetChatCompletionStream returns a stream of chat completions using the given options. - GetChatCompletionStream(context.Context, ChatCompletionOptions, string) (*ChatCompletionResponse, error) + // The httpLogFile parameter, if non-empty, specifies the file to log HTTP requests to. + GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) // GetModelDetails returns the details of the specified model in a particular registry. GetModelDetails(ctx context.Context, registry, modelName, version string) (*ModelDetails, error) // ListModels returns a list of available models. diff --git a/internal/azuremodels/mock_client.go b/internal/azuremodels/mock_client.go index a926b297..60333738 100644 --- a/internal/azuremodels/mock_client.go +++ b/internal/azuremodels/mock_client.go @@ -7,7 +7,7 @@ import ( // MockClient provides a client for interacting with the Azure models API in tests. type MockClient struct { - MockGetChatCompletionStream func(context.Context, ChatCompletionOptions, string) (*ChatCompletionResponse, error) + MockGetChatCompletionStream func(context.Context, ChatCompletionOptions, string, string) (*ChatCompletionResponse, error) MockGetModelDetails func(context.Context, string, string, string) (*ModelDetails, error) MockListModels func(context.Context) ([]*ModelSummary, error) } @@ -15,7 +15,7 @@ type MockClient struct { // NewMockClient returns a new mock client for stubbing out interactions with the models API. func NewMockClient() *MockClient { return &MockClient{ - MockGetChatCompletionStream: func(context.Context, ChatCompletionOptions, string) (*ChatCompletionResponse, error) { + MockGetChatCompletionStream: func(context.Context, ChatCompletionOptions, string, string) (*ChatCompletionResponse, error) { return nil, errors.New("GetChatCompletionStream not implemented") }, MockGetModelDetails: func(context.Context, string, string, string) (*ModelDetails, error) { @@ -28,8 +28,8 @@ func NewMockClient() *MockClient { } // GetChatCompletionStream calls the mocked function for getting a stream of chat completions for the given request. -func (c *MockClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { - return c.MockGetChatCompletionStream(ctx, opt, org) +func (c *MockClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { + return c.MockGetChatCompletionStream(ctx, opt, org, httpLogFile) } // GetModelDetails calls the mocked function for getting the details of the specified model in a particular registry. diff --git a/internal/azuremodels/unauthenticated_client.go b/internal/azuremodels/unauthenticated_client.go index e755f0a8..b465b100 100644 --- a/internal/azuremodels/unauthenticated_client.go +++ b/internal/azuremodels/unauthenticated_client.go @@ -15,7 +15,7 @@ func NewUnauthenticatedClient() *UnauthenticatedClient { } // GetChatCompletionStream returns an error because this functionality requires authentication. -func (c *UnauthenticatedClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { +func (c *UnauthenticatedClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { return nil, errors.New("not authenticated") } From fca40f4c0a19589c73945feff2880ab9fc73f947 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 05:52:59 +0000 Subject: [PATCH 3/7] Add tests for HTTP logging filename feature and remove DEBUG env var usage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- cmd/run/http_log_test.go | 49 ++++++++++++++++++++++ internal/azuremodels/debug_removal_test.go | 36 ++++++++++++++++ 2 files changed, 85 insertions(+) create mode 100644 cmd/run/http_log_test.go create mode 100644 internal/azuremodels/debug_removal_test.go diff --git a/cmd/run/http_log_test.go b/cmd/run/http_log_test.go new file mode 100644 index 00000000..27425480 --- /dev/null +++ b/cmd/run/http_log_test.go @@ -0,0 +1,49 @@ +package run + +import ( + "context" + "testing" + + "github.com/github/gh-models/internal/azuremodels" + "github.com/github/gh-models/internal/sse" + "github.com/github/gh-models/pkg/command" + "github.com/spf13/cobra" + "github.com/stretchr/testify/require" +) + +func TestHttpLogPassthrough(t *testing.T) { + // Test that the httpLog parameter is correctly passed through the call chain + var capturedHttpLog string + + client := azuremodels.NewMockClient() + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + capturedHttpLog = httpLogFile + reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{}) + return &azuremodels.ChatCompletionResponse{Reader: reader}, nil + } + + cfg := command.NewConfig(nil, nil, client, false, 80) + + // Create a command with the http-log flag + cmd := &cobra.Command{} + cmd.Flags().String("http-log", "", "Path to log HTTP requests to (optional)") + cmd.Flags().Set("http-log", "/tmp/test.log") + + // Create handler + handler := newRunCommandHandler(cmd, cfg, []string{}) + + // Test that httpLog is set correctly + require.Equal(t, "/tmp/test.log", handler.httpLog) + + // Test that it's passed to the client call + req := azuremodels.ChatCompletionOptions{ + Model: "test-model", + Messages: []azuremodels.ChatMessage{ + {Role: azuremodels.ChatMessageRoleUser, Content: &[]string{"test"}[0]}, + }, + } + + _, err := handler.getChatCompletionStreamReader(req, "") + require.NoError(t, err) + require.Equal(t, "/tmp/test.log", capturedHttpLog) +} \ No newline at end of file diff --git a/internal/azuremodels/debug_removal_test.go b/internal/azuremodels/debug_removal_test.go new file mode 100644 index 00000000..c8ca3489 --- /dev/null +++ b/internal/azuremodels/debug_removal_test.go @@ -0,0 +1,36 @@ +package azuremodels + +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestHttpLoggingParameterReplacement(t *testing.T) { + // Test that the code no longer references os.Getenv("DEBUG") + // This is a simple test to ensure we removed the DEBUG dependency + + // We'll do a simple code inspection test + // The GetChatCompletionStream method should now use httpLogFile parameter + // instead of checking os.Getenv("DEBUG") + + // Create a mock client to test the interface + client := NewMockClient() + + // Test that the interface accepts the httpLogFile parameter + var capturedHttpLogFile string + client.MockGetChatCompletionStream = func(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { + capturedHttpLogFile = httpLogFile + return &ChatCompletionResponse{}, nil + } + + // Test with empty httpLogFile + _, _ = client.GetChatCompletionStream(nil, ChatCompletionOptions{}, "", "") + require.Equal(t, "", capturedHttpLogFile) + + // Test with specific httpLogFile + testLogFile := "/tmp/test.log" + _, _ = client.GetChatCompletionStream(nil, ChatCompletionOptions{}, "", testLogFile) + require.Equal(t, testLogFile, capturedHttpLogFile) +} \ No newline at end of file From a46680ea8936b6f0a662f586301a814b8d0cf4bc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 06:17:24 +0000 Subject: [PATCH 4/7] Refactor HTTP log to use Context instead of function parameters Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- cmd/eval/eval.go | 12 ++++++---- cmd/eval/eval_test.go | 14 +++++------ cmd/generate/generate.go | 10 +++++--- cmd/generate/generate_test.go | 10 ++++---- cmd/generate/llm.go | 2 +- cmd/generate/pipeline.go | 10 ++++---- cmd/run/http_log_test.go | 13 +++++----- cmd/run/run.go | 20 ++++++++++------ cmd/run/run_test.go | 10 ++++---- internal/azuremodels/azure_client.go | 3 ++- internal/azuremodels/azure_client_test.go | 6 ++--- internal/azuremodels/client.go | 20 ++++++++++++++-- internal/azuremodels/debug_removal_test.go | 24 +++++++++---------- internal/azuremodels/mock_client.go | 8 +++---- .../azuremodels/unauthenticated_client.go | 2 +- 15 files changed, 97 insertions(+), 67 deletions(-) diff --git a/cmd/eval/eval.go b/cmd/eval/eval.go index 209b15a6..a12b7e4a 100644 --- a/cmd/eval/eval.go +++ b/cmd/eval/eval.go @@ -127,10 +127,15 @@ func NewEvalCommand(cfg *command.Config) *cobra.Command { evalFile: evalFile, jsonOutput: jsonOutput, org: org, - httpLog: httpLog, } - err = handler.runEvaluation(cmd.Context()) + ctx := cmd.Context() + // Add HTTP log filename to context if provided + if httpLog != "" { + ctx = azuremodels.WithHTTPLogFile(ctx, httpLog) + } + + err = handler.runEvaluation(ctx) if err == FailedTests { // Cobra by default will show the help message when an error occurs, // which is not what we want for failed evaluations. @@ -153,7 +158,6 @@ type evalCommandHandler struct { evalFile *prompt.File jsonOutput bool org string - httpLog string } func loadEvaluationPromptFile(filePath string) (*prompt.File, error) { @@ -378,7 +382,7 @@ func (h *evalCommandHandler) callModelWithRetry(ctx context.Context, req azuremo const maxRetries = 3 for attempt := 0; attempt <= maxRetries; attempt++ { - resp, err := h.client.GetChatCompletionStream(ctx, req, h.org, h.httpLog) + resp, err := h.client.GetChatCompletionStream(ctx, req, h.org) if err != nil { var rateLimitErr *azuremodels.RateLimitError if errors.As(err, &rateLimitErr) { diff --git a/cmd/eval/eval_test.go b/cmd/eval/eval_test.go index cecb3937..59fc128f 100644 --- a/cmd/eval/eval_test.go +++ b/cmd/eval/eval_test.go @@ -162,7 +162,7 @@ evaluators: cfg := command.NewConfig(out, out, client, true, 100) // Mock a response that returns "4" for the LLM evaluator - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { Choices: []azuremodels.ChatChoice{ @@ -228,7 +228,7 @@ evaluators: client := azuremodels.NewMockClient() // Mock a simple response - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { // Create a mock reader that returns "test response" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -284,7 +284,7 @@ evaluators: client := azuremodels.NewMockClient() // Mock a response that will fail the evaluator - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { Choices: []azuremodels.ChatChoice{ @@ -347,7 +347,7 @@ evaluators: // Mock responses for both test cases callCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { callCount++ var response string if callCount == 1 { @@ -445,7 +445,7 @@ evaluators: require.NoError(t, err) client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { response := "hello world" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -528,7 +528,7 @@ evaluators: require.NoError(t, err) client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { response := "hello world" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ { @@ -590,7 +590,7 @@ evaluators: client := azuremodels.NewMockClient() var capturedRequest azuremodels.ChatCompletionOptions - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { capturedRequest = req response := `{"message": "hello world", "confidence": 0.95}` reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ diff --git a/cmd/generate/generate.go b/cmd/generate/generate.go index 607d47fd..4e2ba4a4 100644 --- a/cmd/generate/generate.go +++ b/cmd/generate/generate.go @@ -17,7 +17,6 @@ type generateCommandHandler struct { client azuremodels.Client options *PromptPexOptions org string - httpLog string } // NewGenerateCommand returns a new command to generate tests using PromptPex. @@ -54,14 +53,19 @@ func NewGenerateCommand(cfg *command.Config) *cobra.Command { // Get http-log flag httpLog, _ := cmd.Flags().GetString("http-log") + ctx := cmd.Context() + // Add HTTP log filename to context if provided + if httpLog != "" { + ctx = azuremodels.WithHTTPLogFile(ctx, httpLog) + } + // Create the command handler handler := &generateCommandHandler{ - ctx: cmd.Context(), + ctx: ctx, cfg: cfg, client: cfg.Client, options: options, org: org, - httpLog: httpLog, } // Create PromptPex context diff --git a/cmd/generate/generate_test.go b/cmd/generate/generate_test.go index 4e37d9c4..c429b73c 100644 --- a/cmd/generate/generate_test.go +++ b/cmd/generate/generate_test.go @@ -207,7 +207,7 @@ messages: // Setup mock client to return error client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { return nil, errors.New("Mock API error") } @@ -241,7 +241,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() callCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { callCount++ var response string @@ -314,7 +314,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content @@ -382,7 +382,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content @@ -451,7 +451,7 @@ messages: // Setup mock client client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { var response string if len(opt.Messages) > 0 && opt.Messages[0].Content != nil { content := *opt.Messages[0].Content diff --git a/cmd/generate/llm.go b/cmd/generate/llm.go index bda9de9e..c4411e82 100644 --- a/cmd/generate/llm.go +++ b/cmd/generate/llm.go @@ -24,7 +24,7 @@ func (h *generateCommandHandler) callModelWithRetry(step string, req azuremodels //nolint:gocritic,revive // TODO defer sp.Stop() - resp, err := h.client.GetChatCompletionStream(ctx, req, h.org, h.httpLog) + resp, err := h.client.GetChatCompletionStream(ctx, req, h.org) if err != nil { var rateLimitErr *azuremodels.RateLimitError if errors.As(err, &rateLimitErr) { diff --git a/cmd/generate/pipeline.go b/cmd/generate/pipeline.go index 3a4ca983..a06a375a 100644 --- a/cmd/generate/pipeline.go +++ b/cmd/generate/pipeline.go @@ -388,7 +388,7 @@ func (h *generateCommandHandler) runSingleTestWithContext(input, modelName strin Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) if err != nil { return "", err } @@ -469,7 +469,7 @@ Compliance:`, rules, output) Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) if err != nil { return EvalResultUnknown, err @@ -510,7 +510,7 @@ Score (0-1):`, metric, output) Temperature: util.Ptr(0.0), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) if err != nil { return 0.0, err @@ -615,7 +615,7 @@ Generate variations in JSON format as an array of objects with "scenario", "test Temperature: util.Ptr(0.5), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) if err != nil { return nil, err @@ -676,7 +676,7 @@ Analysis:`, strings.Join(testSummary, "\n")) Temperature: util.Ptr(0.2), } - response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org, h.httpLog) + response, err := h.client.GetChatCompletionStream(h.ctx, options, h.org) if err != nil { return err diff --git a/cmd/run/http_log_test.go b/cmd/run/http_log_test.go index 27425480..860cfc81 100644 --- a/cmd/run/http_log_test.go +++ b/cmd/run/http_log_test.go @@ -12,12 +12,12 @@ import ( ) func TestHttpLogPassthrough(t *testing.T) { - // Test that the httpLog parameter is correctly passed through the call chain + // Test that the httpLog parameter is correctly passed through the call chain via context var capturedHttpLog string client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { - capturedHttpLog = httpLogFile + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { + capturedHttpLog = azuremodels.HTTPLogFileFromContext(ctx) reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{}) return &azuremodels.ChatCompletionResponse{Reader: reader}, nil } @@ -26,16 +26,17 @@ func TestHttpLogPassthrough(t *testing.T) { // Create a command with the http-log flag cmd := &cobra.Command{} + cmd.SetContext(context.Background()) // Set a context for the command cmd.Flags().String("http-log", "", "Path to log HTTP requests to (optional)") cmd.Flags().Set("http-log", "/tmp/test.log") // Create handler handler := newRunCommandHandler(cmd, cfg, []string{}) - // Test that httpLog is set correctly - require.Equal(t, "/tmp/test.log", handler.httpLog) + // Test that httpLog is correctly stored in context + require.Equal(t, "/tmp/test.log", azuremodels.HTTPLogFileFromContext(handler.ctx)) - // Test that it's passed to the client call + // Test that it's passed to the client call via context req := azuremodels.ChatCompletionOptions{ Model: "test-model", Messages: []azuremodels.ChatMessage{ diff --git a/cmd/run/run.go b/cmd/run/run.go index ce7fddc4..845f5e08 100644 --- a/cmd/run/run.go +++ b/cmd/run/run.go @@ -466,16 +466,22 @@ func parseTemplateVariables(flags *pflag.FlagSet) (map[string]string, error) { } type runCommandHandler struct { - ctx context.Context - cfg *command.Config - client azuremodels.Client - args []string - httpLog string + ctx context.Context + cfg *command.Config + client azuremodels.Client + args []string } func newRunCommandHandler(cmd *cobra.Command, cfg *command.Config, args []string) *runCommandHandler { + ctx := cmd.Context() httpLog, _ := cmd.Flags().GetString("http-log") - return &runCommandHandler{ctx: cmd.Context(), cfg: cfg, client: cfg.Client, args: args, httpLog: httpLog} + + // Add HTTP log filename to context if provided + if httpLog != "" { + ctx = azuremodels.WithHTTPLogFile(ctx, httpLog) + } + + return &runCommandHandler{ctx: ctx, cfg: cfg, client: cfg.Client, args: args} } func (h *runCommandHandler) loadModels() ([]*azuremodels.ModelSummary, error) { @@ -554,7 +560,7 @@ func validateModelName(modelName string, models []*azuremodels.ModelSummary) (st } func (h *runCommandHandler) getChatCompletionStreamReader(req azuremodels.ChatCompletionOptions, org string) (sse.Reader[azuremodels.ChatCompletion], error) { - resp, err := h.client.GetChatCompletionStream(h.ctx, req, org, h.httpLog) + resp, err := h.client.GetChatCompletionStream(h.ctx, req, org) if err != nil { return nil, err } diff --git a/cmd/run/run_test.go b/cmd/run/run_test.go index e9919950..94db2b63 100644 --- a/cmd/run/run_test.go +++ b/cmd/run/run_test.go @@ -44,7 +44,7 @@ func TestRun(t *testing.T) { Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), } getChatCompletionCallCount := 0 - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { getChatCompletionCallCount++ return chatResp, nil } @@ -122,7 +122,7 @@ messages: }, }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -189,7 +189,7 @@ messages: }, }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -281,7 +281,7 @@ messages: }}, } - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { capturedReq = opt return &azuremodels.ChatCompletionResponse{ Reader: sse.NewMockEventReader([]azuremodels.ChatCompletion{chatCompletion}), @@ -367,7 +367,7 @@ messages: } var capturedRequest azuremodels.ChatCompletionOptions - client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org, httpLogFile string) (*azuremodels.ChatCompletionResponse, error) { + client.MockGetChatCompletionStream = func(ctx context.Context, req azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { capturedRequest = req reply := "hello this is a test response" reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{ diff --git a/internal/azuremodels/azure_client.go b/internal/azuremodels/azure_client.go index 0cd9a93a..f14fc903 100644 --- a/internal/azuremodels/azure_client.go +++ b/internal/azuremodels/azure_client.go @@ -45,7 +45,7 @@ func NewAzureClient(httpClient *http.Client, authToken string, cfg *AzureClientC } // GetChatCompletionStream returns a stream of chat completions using the given options. -func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { +func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { // Check for o1 models, which don't support streaming if req.Model == "o1-mini" || req.Model == "o1-preview" || req.Model == "o1" { req.Stream = false @@ -68,6 +68,7 @@ func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompl } // Write request details to specified log file for debugging + httpLogFile := HTTPLogFileFromContext(ctx) if httpLogFile != "" { logFile, err := os.OpenFile(httpLogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err == nil { diff --git a/internal/azuremodels/azure_client_test.go b/internal/azuremodels/azure_client_test.go index 132d11e6..a8b6bf23 100644 --- a/internal/azuremodels/azure_client_test.go +++ b/internal/azuremodels/azure_client_test.go @@ -63,7 +63,7 @@ func TestAzureClient(t *testing.T) { }, } - chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "", "") + chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "") require.NoError(t, err) require.NotNil(t, chatCompletionStreamResp) @@ -139,7 +139,7 @@ func TestAzureClient(t *testing.T) { }, } - chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "", "") + chatCompletionStreamResp, err := client.GetChatCompletionStream(ctx, opts, "") require.NoError(t, err) require.NotNil(t, chatCompletionStreamResp) @@ -181,7 +181,7 @@ func TestAzureClient(t *testing.T) { Messages: []ChatMessage{{Role: "user", Content: util.Ptr("Tell me a story, test model.")}}, } - chatCompletionResp, err := client.GetChatCompletionStream(ctx, opts, "", "") + chatCompletionResp, err := client.GetChatCompletionStream(ctx, opts, "") require.Error(t, err) require.Nil(t, chatCompletionResp) diff --git a/internal/azuremodels/client.go b/internal/azuremodels/client.go index 053104b5..582b6743 100644 --- a/internal/azuremodels/client.go +++ b/internal/azuremodels/client.go @@ -2,11 +2,27 @@ package azuremodels import "context" +// httpLogFileKey is the context key for the HTTP log filename +type httpLogFileKey struct{} + +// WithHTTPLogFile returns a new context with the HTTP log filename attached +func WithHTTPLogFile(ctx context.Context, httpLogFile string) context.Context { + return context.WithValue(ctx, httpLogFileKey{}, httpLogFile) +} + +// HTTPLogFileFromContext returns the HTTP log filename from the context, if any +func HTTPLogFileFromContext(ctx context.Context) string { + if httpLogFile, ok := ctx.Value(httpLogFileKey{}).(string); ok { + return httpLogFile + } + return "" +} + // Client represents a client for interacting with an API about models. type Client interface { // GetChatCompletionStream returns a stream of chat completions using the given options. - // The httpLogFile parameter, if non-empty, specifies the file to log HTTP requests to. - GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) + // HTTP logging configuration is extracted from the context if present. + GetChatCompletionStream(ctx context.Context, req ChatCompletionOptions, org string) (*ChatCompletionResponse, error) // GetModelDetails returns the details of the specified model in a particular registry. GetModelDetails(ctx context.Context, registry, modelName, version string) (*ModelDetails, error) // ListModels returns a list of available models. diff --git a/internal/azuremodels/debug_removal_test.go b/internal/azuremodels/debug_removal_test.go index c8ca3489..ebec4591 100644 --- a/internal/azuremodels/debug_removal_test.go +++ b/internal/azuremodels/debug_removal_test.go @@ -8,29 +8,27 @@ import ( ) func TestHttpLoggingParameterReplacement(t *testing.T) { - // Test that the code no longer references os.Getenv("DEBUG") - // This is a simple test to ensure we removed the DEBUG dependency - - // We'll do a simple code inspection test - // The GetChatCompletionStream method should now use httpLogFile parameter - // instead of checking os.Getenv("DEBUG") + // Test that HTTP logging now uses context instead of function parameters + // This test ensures we moved HTTP log configuration to context // Create a mock client to test the interface client := NewMockClient() - // Test that the interface accepts the httpLogFile parameter + // Test that the interface accepts context and extracts HTTP log filename var capturedHttpLogFile string - client.MockGetChatCompletionStream = func(ctx context.Context, req ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { - capturedHttpLogFile = httpLogFile + client.MockGetChatCompletionStream = func(ctx context.Context, req ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { + capturedHttpLogFile = HTTPLogFileFromContext(ctx) return &ChatCompletionResponse{}, nil } - // Test with empty httpLogFile - _, _ = client.GetChatCompletionStream(nil, ChatCompletionOptions{}, "", "") + // Test with context without HTTP log file + ctx := context.Background() + _, _ = client.GetChatCompletionStream(ctx, ChatCompletionOptions{}, "") require.Equal(t, "", capturedHttpLogFile) - // Test with specific httpLogFile + // Test with context containing HTTP log file testLogFile := "/tmp/test.log" - _, _ = client.GetChatCompletionStream(nil, ChatCompletionOptions{}, "", testLogFile) + ctxWithLog := WithHTTPLogFile(ctx, testLogFile) + _, _ = client.GetChatCompletionStream(ctxWithLog, ChatCompletionOptions{}, "") require.Equal(t, testLogFile, capturedHttpLogFile) } \ No newline at end of file diff --git a/internal/azuremodels/mock_client.go b/internal/azuremodels/mock_client.go index 60333738..a926b297 100644 --- a/internal/azuremodels/mock_client.go +++ b/internal/azuremodels/mock_client.go @@ -7,7 +7,7 @@ import ( // MockClient provides a client for interacting with the Azure models API in tests. type MockClient struct { - MockGetChatCompletionStream func(context.Context, ChatCompletionOptions, string, string) (*ChatCompletionResponse, error) + MockGetChatCompletionStream func(context.Context, ChatCompletionOptions, string) (*ChatCompletionResponse, error) MockGetModelDetails func(context.Context, string, string, string) (*ModelDetails, error) MockListModels func(context.Context) ([]*ModelSummary, error) } @@ -15,7 +15,7 @@ type MockClient struct { // NewMockClient returns a new mock client for stubbing out interactions with the models API. func NewMockClient() *MockClient { return &MockClient{ - MockGetChatCompletionStream: func(context.Context, ChatCompletionOptions, string, string) (*ChatCompletionResponse, error) { + MockGetChatCompletionStream: func(context.Context, ChatCompletionOptions, string) (*ChatCompletionResponse, error) { return nil, errors.New("GetChatCompletionStream not implemented") }, MockGetModelDetails: func(context.Context, string, string, string) (*ModelDetails, error) { @@ -28,8 +28,8 @@ func NewMockClient() *MockClient { } // GetChatCompletionStream calls the mocked function for getting a stream of chat completions for the given request. -func (c *MockClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { - return c.MockGetChatCompletionStream(ctx, opt, org, httpLogFile) +func (c *MockClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { + return c.MockGetChatCompletionStream(ctx, opt, org) } // GetModelDetails calls the mocked function for getting the details of the specified model in a particular registry. diff --git a/internal/azuremodels/unauthenticated_client.go b/internal/azuremodels/unauthenticated_client.go index b465b100..e755f0a8 100644 --- a/internal/azuremodels/unauthenticated_client.go +++ b/internal/azuremodels/unauthenticated_client.go @@ -15,7 +15,7 @@ func NewUnauthenticatedClient() *UnauthenticatedClient { } // GetChatCompletionStream returns an error because this functionality requires authentication. -func (c *UnauthenticatedClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org, httpLogFile string) (*ChatCompletionResponse, error) { +func (c *UnauthenticatedClient) GetChatCompletionStream(ctx context.Context, opt ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { return nil, errors.New("not authenticated") } From 3cdc67e220b07d8d98a1e44aa70db1bcef655a57 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 07:41:19 +0000 Subject: [PATCH 5/7] Group HTTP log writes into single fprintf call for better performance Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- internal/azuremodels/azure_client.go | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/internal/azuremodels/azure_client.go b/internal/azuremodels/azure_client.go index f14fc903..ac4c8ef9 100644 --- a/internal/azuremodels/azure_client.go +++ b/internal/azuremodels/azure_client.go @@ -73,13 +73,8 @@ func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompl logFile, err := os.OpenFile(httpLogFile, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) if err == nil { defer logFile.Close() - fmt.Fprintf(logFile, "### %s\n", time.Now().Format(time.RFC3339)) - fmt.Fprintf(logFile, "POST %s\n", inferenceURL) - fmt.Fprintf(logFile, "Authorization: Bearer {{$processEnv GITHUB_TOKEN}}\n") - fmt.Fprintf(logFile, "Content-Type: application/json\n") - fmt.Fprintf(logFile, "x-ms-useragent: github-cli-models\n") - fmt.Fprintf(logFile, "x-ms-user-agent: github-cli-models\n") - fmt.Fprintf(logFile, "\n%s\n\n", string(bodyBytes)) + fmt.Fprintf(logFile, "### %s\nPOST %s\nAuthorization: Bearer {{$processEnv GITHUB_TOKEN}}\nContent-Type: application/json\nx-ms-useragent: github-cli-models\nx-ms-user-agent: github-cli-models\n\n%s\n\n", + time.Now().Format(time.RFC3339), inferenceURL, string(bodyBytes)) } } From d19f533bdc35f5abc412a22bf425dd0597bd4eb2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 24 Jul 2025 07:48:24 +0000 Subject: [PATCH 6/7] Remove test files as requested Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- cmd/run/http_log_test.go | 50 ---------------------- internal/azuremodels/debug_removal_test.go | 34 --------------- 2 files changed, 84 deletions(-) delete mode 100644 cmd/run/http_log_test.go delete mode 100644 internal/azuremodels/debug_removal_test.go diff --git a/cmd/run/http_log_test.go b/cmd/run/http_log_test.go deleted file mode 100644 index 860cfc81..00000000 --- a/cmd/run/http_log_test.go +++ /dev/null @@ -1,50 +0,0 @@ -package run - -import ( - "context" - "testing" - - "github.com/github/gh-models/internal/azuremodels" - "github.com/github/gh-models/internal/sse" - "github.com/github/gh-models/pkg/command" - "github.com/spf13/cobra" - "github.com/stretchr/testify/require" -) - -func TestHttpLogPassthrough(t *testing.T) { - // Test that the httpLog parameter is correctly passed through the call chain via context - var capturedHttpLog string - - client := azuremodels.NewMockClient() - client.MockGetChatCompletionStream = func(ctx context.Context, opt azuremodels.ChatCompletionOptions, org string) (*azuremodels.ChatCompletionResponse, error) { - capturedHttpLog = azuremodels.HTTPLogFileFromContext(ctx) - reader := sse.NewMockEventReader([]azuremodels.ChatCompletion{}) - return &azuremodels.ChatCompletionResponse{Reader: reader}, nil - } - - cfg := command.NewConfig(nil, nil, client, false, 80) - - // Create a command with the http-log flag - cmd := &cobra.Command{} - cmd.SetContext(context.Background()) // Set a context for the command - cmd.Flags().String("http-log", "", "Path to log HTTP requests to (optional)") - cmd.Flags().Set("http-log", "/tmp/test.log") - - // Create handler - handler := newRunCommandHandler(cmd, cfg, []string{}) - - // Test that httpLog is correctly stored in context - require.Equal(t, "/tmp/test.log", azuremodels.HTTPLogFileFromContext(handler.ctx)) - - // Test that it's passed to the client call via context - req := azuremodels.ChatCompletionOptions{ - Model: "test-model", - Messages: []azuremodels.ChatMessage{ - {Role: azuremodels.ChatMessageRoleUser, Content: &[]string{"test"}[0]}, - }, - } - - _, err := handler.getChatCompletionStreamReader(req, "") - require.NoError(t, err) - require.Equal(t, "/tmp/test.log", capturedHttpLog) -} \ No newline at end of file diff --git a/internal/azuremodels/debug_removal_test.go b/internal/azuremodels/debug_removal_test.go deleted file mode 100644 index ebec4591..00000000 --- a/internal/azuremodels/debug_removal_test.go +++ /dev/null @@ -1,34 +0,0 @@ -package azuremodels - -import ( - "context" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestHttpLoggingParameterReplacement(t *testing.T) { - // Test that HTTP logging now uses context instead of function parameters - // This test ensures we moved HTTP log configuration to context - - // Create a mock client to test the interface - client := NewMockClient() - - // Test that the interface accepts context and extracts HTTP log filename - var capturedHttpLogFile string - client.MockGetChatCompletionStream = func(ctx context.Context, req ChatCompletionOptions, org string) (*ChatCompletionResponse, error) { - capturedHttpLogFile = HTTPLogFileFromContext(ctx) - return &ChatCompletionResponse{}, nil - } - - // Test with context without HTTP log file - ctx := context.Background() - _, _ = client.GetChatCompletionStream(ctx, ChatCompletionOptions{}, "") - require.Equal(t, "", capturedHttpLogFile) - - // Test with context containing HTTP log file - testLogFile := "/tmp/test.log" - ctxWithLog := WithHTTPLogFile(ctx, testLogFile) - _, _ = client.GetChatCompletionStream(ctxWithLog, ChatCompletionOptions{}, "") - require.Equal(t, testLogFile, capturedHttpLogFile) -} \ No newline at end of file From 7903afab145fe95b664e37b5d5f76d7c35c0aa23 Mon Sep 17 00:00:00 2001 From: Peli de Halleux Date: Thu, 24 Jul 2025 00:49:04 -0700 Subject: [PATCH 7/7] Update internal/azuremodels/azure_client.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- internal/azuremodels/azure_client.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/azuremodels/azure_client.go b/internal/azuremodels/azure_client.go index ac4c8ef9..7ff082a3 100644 --- a/internal/azuremodels/azure_client.go +++ b/internal/azuremodels/azure_client.go @@ -74,7 +74,8 @@ func (c *AzureClient) GetChatCompletionStream(ctx context.Context, req ChatCompl if err == nil { defer logFile.Close() fmt.Fprintf(logFile, "### %s\nPOST %s\nAuthorization: Bearer {{$processEnv GITHUB_TOKEN}}\nContent-Type: application/json\nx-ms-useragent: github-cli-models\nx-ms-user-agent: github-cli-models\n\n%s\n\n", - time.Now().Format(time.RFC3339), inferenceURL, string(bodyBytes)) + const logFormat = "### %s\nPOST %s\nAuthorization: Bearer {{$processEnv GITHUB_TOKEN}}\nContent-Type: application/json\nx-ms-useragent: github-cli-models\nx-ms-user-agent: github-cli-models\n\n%s\n\n" + fmt.Fprintf(logFile, logFormat, time.Now().Format(time.RFC3339), inferenceURL, string(bodyBytes)) } }