diff --git a/provider/copilotprovider/copilot.go b/provider/copilotprovider/copilot.go index 1f6d9ee8..96b4b989 100644 --- a/provider/copilotprovider/copilot.go +++ b/provider/copilotprovider/copilot.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "iter" + "log/slog" "os" "path/filepath" "slices" @@ -236,6 +237,7 @@ func (p *provider) sessionConfig(streaming bool, options []agent.Option) copilot cfg.Streaming = copilot.Bool(streaming) cfg.SystemMessage = systemMessageWithInstructions(cfg.SystemMessage, slices.Collect(agent.AllOptions(options, agent.WithInstructions))) cfg.Tools = append(cfg.Tools, copilotTools(options)...) + cfg.Hooks = sessionHooksWithApprovalRequests(cfg.Tools, cfg.Hooks) return cfg } @@ -244,6 +246,7 @@ func (p *provider) resumeSessionConfig(streaming bool, options []agent.Option) c cfg.Streaming = copilot.Bool(streaming) cfg.SystemMessage = systemMessageWithInstructions(cfg.SystemMessage, slices.Collect(agent.AllOptions(options, agent.WithInstructions))) cfg.Tools = append(cfg.Tools, copilotTools(options)...) + cfg.Hooks = sessionHooksWithApprovalRequests(cfg.Tools, cfg.Hooks) return cfg } @@ -252,6 +255,7 @@ func copySessionConfig(source *copilot.SessionConfig) copilot.SessionConfig { return copilot.SessionConfig{Streaming: copilot.Bool(true)} } clone := *source + clone.Tools = slices.Clone(source.Tools) clone.Streaming = copyBoolDefaultTrue(source.Streaming) return clone } @@ -263,7 +267,7 @@ func copyResumeSessionConfig(source *copilot.SessionConfig) copilot.ResumeSessio return copilot.ResumeSessionConfig{ Model: source.Model, ReasoningEffort: source.ReasoningEffort, - Tools: source.Tools, + Tools: slices.Clone(source.Tools), SystemMessage: source.SystemMessage, AvailableTools: source.AvailableTools, ExcludedTools: source.ExcludedTools, @@ -290,6 +294,73 @@ func copyBoolDefaultTrue(source *bool) *bool { return &value } +func sessionHooksWithApprovalRequests(tools []copilot.Tool, hooks *copilot.SessionHooks) *copilot.SessionHooks { + var names map[string]struct{} + approvalRequiredNames := func() map[string]struct{} { + if names == nil { + names = approvalRequiredToolNames(tools) + } + return names + } + if hooks != nil && hooks.OnPreToolUse != nil { + logApprovalGatingSkipped(approvalRequiredNames()) + return hooks + } + names = approvalRequiredNames() + if len(names) == 0 { + return hooks + } + clone := cloneSessionHooks(hooks) + clone.OnPreToolUse = func(input copilot.PreToolUseHookInput, _ copilot.HookInvocation) (*copilot.PreToolUseHookOutput, error) { + if _, ok := names[input.ToolName]; !ok { + return nil, nil + } + return &copilot.PreToolUseHookOutput{ + PermissionDecision: "ask", + PermissionDecisionReason: fmt.Sprintf("Tool %q requires approval before it can run.", input.ToolName), + }, nil + } + return clone +} + +func approvalRequiredToolNames(tools []copilot.Tool) map[string]struct{} { + var names map[string]struct{} + for _, tl := range tools { + if tl.Name == "" || tl.SkipPermission { + continue + } + if names == nil { + names = make(map[string]struct{}) + } + names[tl.Name] = struct{}{} + } + return names +} + +func logApprovalGatingSkipped(names map[string]struct{}) { + if len(names) == 0 { + return + } + toolNames := make([]string, 0, len(names)) + for name := range names { + toolNames = append(toolNames, name) + } + slices.Sort(toolNames) + slog.Warn( + "A custom OnPreToolUse hook is configured, so approval-required tools will not be automatically gated.", + "approvalRequiredToolCount", len(toolNames), + "approvalRequiredTools", strings.Join(toolNames, ", "), + ) +} + +func cloneSessionHooks(source *copilot.SessionHooks) *copilot.SessionHooks { + if source == nil { + return &copilot.SessionHooks{} + } + clone := *source + return &clone +} + func systemMessageWithInstructions(base *copilot.SystemMessageConfig, instructions []string) *copilot.SystemMessageConfig { instructions = compactInstructions(instructions) if len(instructions) == 0 { diff --git a/provider/copilotprovider/copilot_internal_test.go b/provider/copilotprovider/copilot_internal_test.go new file mode 100644 index 00000000..22056a67 --- /dev/null +++ b/provider/copilotprovider/copilot_internal_test.go @@ -0,0 +1,163 @@ +// Copyright (c) Microsoft. All rights reserved. + +package copilotprovider + +import ( + "bytes" + "context" + "log/slog" + "strings" + "testing" + + copilot "github.com/github/copilot-sdk/go" + "github.com/microsoft/agent-framework-go/agent" + "github.com/microsoft/agent-framework-go/tool" + "github.com/microsoft/agent-framework-go/tool/functool" +) + +func TestSessionConfig_WithApprovalRequiredTool_InstallsAskPreToolUseHook(t *testing.T) { + dangerousTool := tool.ApprovalRequiredFunc(testFuncTool(t, "dangerous")) + plainTool := testFuncTool(t, "plain") + p := &provider{} + + cfg := p.sessionConfig(true, []agent.Option{ + agent.WithTool(dangerousTool), + agent.WithTool(plainTool), + }) + + if cfg.Hooks == nil || cfg.Hooks.OnPreToolUse == nil { + t.Fatal("OnPreToolUse hook was not installed") + } + + dangerousDecision, err := cfg.Hooks.OnPreToolUse(copilot.PreToolUseHookInput{ToolName: "dangerous"}, copilot.HookInvocation{}) + if err != nil { + t.Fatalf("OnPreToolUse(dangerous): %v", err) + } + if dangerousDecision == nil || dangerousDecision.PermissionDecision != "ask" { + t.Fatalf("dangerous permission decision = %#v, want ask", dangerousDecision) + } + + plainDecision, err := cfg.Hooks.OnPreToolUse(copilot.PreToolUseHookInput{ToolName: "plain"}, copilot.HookInvocation{}) + if err != nil { + t.Fatalf("OnPreToolUse(plain): %v", err) + } + if plainDecision != nil { + t.Fatalf("plain permission decision = %#v, want nil", plainDecision) + } +} + +func TestSessionConfig_WithSessionConfigToolRequiringApproval_InstallsHookWithoutMutatingSource(t *testing.T) { + source := &copilot.SessionConfig{ + Tools: []copilot.Tool{{Name: "dangerous"}}, + Hooks: &copilot.SessionHooks{}, + } + p := &provider{cfg: AgentConfig{SessionConfig: source}} + + cfg := p.sessionConfig(true, []agent.Option{agent.WithTool(testFuncTool(t, "plain"))}) + + if source.Hooks.OnPreToolUse != nil { + t.Fatal("source hooks were mutated") + } + if len(source.Tools) != 1 { + t.Fatalf("source tools length = %d, want 1", len(source.Tools)) + } + if len(cfg.Tools) != 2 { + t.Fatalf("session tools length = %d, want 2", len(cfg.Tools)) + } + if cfg.Hooks == nil || cfg.Hooks == source.Hooks { + t.Fatal("session hooks were not cloned") + } + decision, err := cfg.Hooks.OnPreToolUse(copilot.PreToolUseHookInput{ToolName: "dangerous"}, copilot.HookInvocation{}) + if err != nil { + t.Fatalf("OnPreToolUse(dangerous): %v", err) + } + if decision == nil || decision.PermissionDecision != "ask" { + t.Fatalf("dangerous permission decision = %#v, want ask", decision) + } +} + +func TestSessionConfig_WithExistingPreToolUseHook_PreservesCallerHook(t *testing.T) { + expected := &copilot.PreToolUseHookOutput{PermissionDecision: "allow"} + source := &copilot.SessionConfig{ + Hooks: &copilot.SessionHooks{ + OnPreToolUse: func(copilot.PreToolUseHookInput, copilot.HookInvocation) (*copilot.PreToolUseHookOutput, error) { + return expected, nil + }, + }, + } + p := &provider{cfg: AgentConfig{SessionConfig: source}} + + cfg := p.sessionConfig(true, []agent.Option{agent.WithTool(tool.ApprovalRequiredFunc(testFuncTool(t, "dangerous")))}) + + if cfg.Hooks == nil || cfg.Hooks.OnPreToolUse == nil { + t.Fatal("OnPreToolUse hook is nil") + } + got, err := cfg.Hooks.OnPreToolUse(copilot.PreToolUseHookInput{ToolName: "dangerous"}, copilot.HookInvocation{}) + if err != nil { + t.Fatalf("OnPreToolUse: %v", err) + } + if got != expected { + t.Fatalf("permission decision = %#v, want %#v", got, expected) + } +} + +func TestSessionConfig_WithExistingPreToolUseHookAndApprovalTools_LogsWarning(t *testing.T) { + var logs bytes.Buffer + defaultLogger := slog.Default() + slog.SetDefault(slog.New(slog.NewTextHandler(&logs, &slog.HandlerOptions{Level: slog.LevelWarn}))) + t.Cleanup(func() { + slog.SetDefault(defaultLogger) + }) + + source := &copilot.SessionConfig{ + Hooks: &copilot.SessionHooks{ + OnPreToolUse: func(copilot.PreToolUseHookInput, copilot.HookInvocation) (*copilot.PreToolUseHookOutput, error) { + return nil, nil + }, + }, + } + p := &provider{cfg: AgentConfig{SessionConfig: source}} + + _ = p.sessionConfig(true, []agent.Option{ + agent.WithTool(tool.ApprovalRequiredFunc(testFuncTool(t, "dangerous-a"))), + agent.WithTool(tool.ApprovalRequiredFunc(testFuncTool(t, "dangerous-b"))), + }) + + logOutput := logs.String() + if !strings.Contains(logOutput, "not be automatically gated") { + t.Fatalf("expected warning log for skipped approval gating, got %q", logOutput) + } + if !strings.Contains(logOutput, "approvalRequiredToolCount=2") { + t.Fatalf("expected tool count in warning log, got %q", logOutput) + } + if !strings.Contains(logOutput, "dangerous-a, dangerous-b") { + t.Fatalf("expected tool names in warning log, got %q", logOutput) + } +} + +func TestResumeSessionConfig_WithApprovalRequiredTool_InstallsAskPreToolUseHook(t *testing.T) { + p := &provider{} + + cfg := p.resumeSessionConfig(true, []agent.Option{ + agent.WithTool(tool.ApprovalRequiredFunc(testFuncTool(t, "dangerous"))), + }) + + if cfg.Hooks == nil || cfg.Hooks.OnPreToolUse == nil { + t.Fatal("OnPreToolUse hook was not installed") + } + decision, err := cfg.Hooks.OnPreToolUse(copilot.PreToolUseHookInput{ToolName: "dangerous"}, copilot.HookInvocation{}) + if err != nil { + t.Fatalf("OnPreToolUse: %v", err) + } + if decision == nil || decision.PermissionDecision != "ask" { + t.Fatalf("dangerous permission decision = %#v, want ask", decision) + } +} + +func testFuncTool(t *testing.T, name string) tool.FuncTool { + t.Helper() + return functool.MustNew( + functool.Config{Name: name, Description: name + " description"}, + func(context.Context, struct{}) (string, error) { return "ok", nil }, + ) +}