diff --git a/internal/agent/agentic_fetch_tool.go b/internal/agent/agentic_fetch_tool.go index 89d3535720f8452111f12f4df4eb691e39253bed..472b0f2ed42cfb1ca0e50809032e21de9fb729b8 100644 --- a/internal/agent/agentic_fetch_tool.go +++ b/internal/agent/agentic_fetch_tool.go @@ -93,7 +93,10 @@ func (c *coordinator) agenticFetchTool(_ context.Context, client *http.Client) ( if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -228,6 +231,6 @@ func (c *coordinator) agenticFetchTool(_ context.Context, client *http.Client) ( return fantasy.ToolResponse{}, fmt.Errorf("error saving parent session: %s", err) } - return fantasy.NewTextResponse(result.Response.Content.Text()), nil + return fantasy.NewTextResponse(p.AppendCommentary(result.Response.Content.Text())), nil }), nil } diff --git a/internal/agent/tools/bash.go b/internal/agent/tools/bash.go index ca3612e091f23235688a2a40006469e39093d6a5..2aba3e45d95086bcb92dd382fe3e1c2ca4218d19 100644 --- a/internal/agent/tools/bash.go +++ b/internal/agent/tools/bash.go @@ -214,8 +214,10 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution if sessionID == "" { return fantasy.ToolResponse{}, fmt.Errorf("session ID is required for executing shell command") } + var permResult permission.PermissionResult if !isSafeReadOnly { - p, err := permissions.Request(ctx, + var err error + permResult, err = permissions.Request(ctx, permission.CreatePermissionRequest{ SessionID: sessionID, Path: execWorkingDir, @@ -229,7 +231,10 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !permResult.Granted { + if permResult.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(permResult.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } } @@ -270,10 +275,10 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution WorkingDirectory: bgShell.WorkingDir, } if stdout == "" { - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(BashNoOutput), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(BashNoOutput)), metadata), nil } stdout += fmt.Sprintf("\n\n%s", normalizeWorkingDir(bgShell.WorkingDir)) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(stdout), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(stdout)), metadata), nil } // Still running after fast-failure check - return as background job @@ -286,7 +291,7 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution ShellID: bgShell.ID, } response := fmt.Sprintf("Background shell started with ID: %s\n\nUse job_output tool to view output or job_kill to terminate.", bgShell.ID) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(response), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(response)), metadata), nil } // Start synchronous execution with auto-background support @@ -351,10 +356,10 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution WorkingDirectory: bgShell.WorkingDir, } if stdout == "" { - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(BashNoOutput), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(BashNoOutput)), metadata), nil } stdout += fmt.Sprintf("\n\n%s", normalizeWorkingDir(bgShell.WorkingDir)) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(stdout), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(stdout)), metadata), nil } // Still running - keep as background job @@ -367,7 +372,7 @@ func NewBashTool(permissions permission.Service, workingDir string, attribution ShellID: bgShell.ID, } response := fmt.Sprintf("Command is taking longer than expected and has been moved to background.\n\nBackground shell ID: %s\n\nUse job_output tool to view output or job_kill to terminate.", bgShell.ID) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(response), metadata), nil + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(permResult.AppendCommentary(response)), metadata), nil }) } diff --git a/internal/agent/tools/download.go b/internal/agent/tools/download.go index 8f3f224b9e5647911d3c7e1cc5a668eea18b1785..ccb27dceac0b91169d02d6892c962a0bb5f173c7 100644 --- a/internal/agent/tools/download.go +++ b/internal/agent/tools/download.go @@ -83,7 +83,10 @@ func NewDownloadTool(permissions permission.Service, workingDir string, client * if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -142,6 +145,6 @@ func NewDownloadTool(permissions permission.Service, workingDir string, client * responseMsg += fmt.Sprintf(" (Content-Type: %s)", contentType) } - return fantasy.NewTextResponse(responseMsg), nil + return fantasy.NewTextResponse(p.AppendCommentary(responseMsg)), nil }) } diff --git a/internal/agent/tools/edit.go b/internal/agent/tools/edit.go index 2c9b15abfe148fb881ee90f75f207c1134776281..ffca0db82879fbfdace66a3931d6ef52018d10c8 100644 --- a/internal/agent/tools/edit.go +++ b/internal/agent/tools/edit.go @@ -145,7 +145,10 @@ func createNewFile(edit editContext, filePath, content string, call fantasy.Tool if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -172,7 +175,7 @@ func createNewFile(edit editContext, filePath, content string, call fantasy.Tool filetracker.RecordRead(filePath) return fantasy.WithResponseMetadata( - fantasy.NewTextResponse("File created: "+filePath), + fantasy.NewTextResponse(p.AppendCommentary("File created: "+filePath)), EditResponseMetadata{ OldContent: "", NewContent: content, @@ -266,7 +269,10 @@ func deleteContent(edit editContext, filePath, oldString string, replaceAll bool if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -305,7 +311,7 @@ func deleteContent(edit editContext, filePath, oldString string, replaceAll bool filetracker.RecordRead(filePath) return fantasy.WithResponseMetadata( - fantasy.NewTextResponse("Content deleted from file: "+filePath), + fantasy.NewTextResponse(p.AppendCommentary("Content deleted from file: "+filePath)), EditResponseMetadata{ OldContent: oldContent, NewContent: newContent, @@ -398,7 +404,10 @@ func replaceContent(edit editContext, filePath, oldString, newString string, rep if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -437,7 +446,7 @@ func replaceContent(edit editContext, filePath, oldString, newString string, rep filetracker.RecordRead(filePath) return fantasy.WithResponseMetadata( - fantasy.NewTextResponse("Content replaced in file: "+filePath), + fantasy.NewTextResponse(p.AppendCommentary("Content replaced in file: "+filePath)), EditResponseMetadata{ OldContent: oldContent, NewContent: newContent, diff --git a/internal/agent/tools/fetch.go b/internal/agent/tools/fetch.go index fdb63f057958e5e5a67affe0783a452c27febf41..9f1921e3aaf1787883555a273e1cce9b3cbcd583 100644 --- a/internal/agent/tools/fetch.go +++ b/internal/agent/tools/fetch.go @@ -69,7 +69,10 @@ func NewFetchTool(permissions permission.Service, workingDir string, client *htt if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -165,7 +168,7 @@ func NewFetchTool(permissions permission.Service, workingDir string, client *htt content += fmt.Sprintf("\n\n[Content truncated to %d bytes]", MaxReadSize) } - return fantasy.NewTextResponse(content), nil + return fantasy.NewTextResponse(p.AppendCommentary(content)), nil }) } diff --git a/internal/agent/tools/ls.go b/internal/agent/tools/ls.go index 20bb1bad4c2d92bb02e564045d4553206ffd12a1..504a2f4f0e2ed2e196a69f0f70fb4d80f3c44928 100644 --- a/internal/agent/tools/ls.go +++ b/internal/agent/tools/ls.go @@ -78,6 +78,7 @@ func NewLsTool(permissions permission.Service, workingDir string, lsConfig confi return fantasy.NewTextErrorResponse(fmt.Sprintf("error resolving search path: %v", err)), nil } + var permResult permission.PermissionResult relPath, err := filepath.Rel(absWorkingDir, absSearchPath) if err != nil || strings.HasPrefix(relPath, "..") { // Directory is outside working directory, request permission @@ -86,7 +87,7 @@ func NewLsTool(permissions permission.Service, workingDir string, lsConfig confi return fantasy.ToolResponse{}, fmt.Errorf("session ID is required for accessing directories outside working directory") } - granted, err := permissions.Request(ctx, + permResult, err = permissions.Request(ctx, permission.CreatePermissionRequest{ SessionID: sessionID, Path: absSearchPath, @@ -100,7 +101,10 @@ func NewLsTool(permissions permission.Service, workingDir string, lsConfig confi if err != nil { return fantasy.ToolResponse{}, err } - if !granted { + if !permResult.Granted { + if permResult.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(permResult.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } } @@ -111,7 +115,7 @@ func NewLsTool(permissions permission.Service, workingDir string, lsConfig confi } return fantasy.WithResponseMetadata( - fantasy.NewTextResponse(output), + fantasy.NewTextResponse(permResult.AppendCommentary(output)), metadata, ), nil }) diff --git a/internal/agent/tools/mcp-tools.go b/internal/agent/tools/mcp-tools.go index fa55f03728639a09e6bd2f150338238d30120883..16ad62ec9a60635fd0a529c764cd189f64289e9f 100644 --- a/internal/agent/tools/mcp-tools.go +++ b/internal/agent/tools/mcp-tools.go @@ -103,7 +103,10 @@ func (m *Tool) Run(ctx context.Context, params fantasy.ToolCall) (fantasy.ToolRe if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -125,9 +128,9 @@ func (m *Tool) Run(ctx context.Context, params fantasy.ToolCall) (fantasy.ToolRe } else { response = fantasy.NewMediaResponse(result.Data, result.MediaType) } - response.Content = result.Content + response.Content = p.AppendCommentary(result.Content) return response, nil default: - return fantasy.NewTextResponse(result.Content), nil + return fantasy.NewTextResponse(p.AppendCommentary(result.Content)), nil } } diff --git a/internal/agent/tools/multiedit.go b/internal/agent/tools/multiedit.go index 0640228d23230e6a49d8e1405f371c099031fbf7..b80020924da94abdb7783af4458e50f2e8994bd9 100644 --- a/internal/agent/tools/multiedit.go +++ b/internal/agent/tools/multiedit.go @@ -189,7 +189,10 @@ func processMultiEditWithCreation(edit editContext, params MultiEditParams, call if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -221,7 +224,7 @@ func processMultiEditWithCreation(edit editContext, params MultiEditParams, call } return fantasy.WithResponseMetadata( - fantasy.NewTextResponse(message), + fantasy.NewTextResponse(p.AppendCommentary(message)), MultiEditResponseMetadata{ OldContent: "", NewContent: currentContent, @@ -333,7 +336,10 @@ func processMultiEditExistingFile(edit editContext, params MultiEditParams, call if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -380,7 +386,7 @@ func processMultiEditExistingFile(edit editContext, params MultiEditParams, call } return fantasy.WithResponseMetadata( - fantasy.NewTextResponse(message), + fantasy.NewTextResponse(p.AppendCommentary(message)), MultiEditResponseMetadata{ OldContent: oldContent, NewContent: currentContent, diff --git a/internal/agent/tools/multiedit_test.go b/internal/agent/tools/multiedit_test.go index b6d575435e63dcd62a4dc9a7efb76cf13c14ad05..d413bf07e25d88f9fae2993929525500c95e9699 100644 --- a/internal/agent/tools/multiedit_test.go +++ b/internal/agent/tools/multiedit_test.go @@ -19,15 +19,15 @@ type mockPermissionService struct { *pubsub.Broker[permission.PermissionRequest] } -func (m *mockPermissionService) Request(ctx context.Context, req permission.CreatePermissionRequest) (bool, error) { - return true, nil +func (m *mockPermissionService) Request(ctx context.Context, req permission.CreatePermissionRequest) (permission.PermissionResult, error) { + return permission.PermissionResult{Granted: true}, nil } -func (m *mockPermissionService) Grant(req permission.PermissionRequest) {} +func (m *mockPermissionService) Grant(req permission.PermissionRequest, message string) {} -func (m *mockPermissionService) Deny(req permission.PermissionRequest) {} +func (m *mockPermissionService) Deny(req permission.PermissionRequest, message string) {} -func (m *mockPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *mockPermissionService) GrantPersistent(req permission.PermissionRequest, message string) {} func (m *mockPermissionService) AutoApproveSession(sessionID string) {} diff --git a/internal/agent/tools/view.go b/internal/agent/tools/view.go index 35865cf43f7c587d60764b3ed177374940bbe2dc..31541e9fa5b2ff241f04dc6fe9e313d49beb63ff 100644 --- a/internal/agent/tools/view.go +++ b/internal/agent/tools/view.go @@ -74,6 +74,7 @@ func NewViewTool(lspClients *csync.Map[string, *lsp.Client], permissions permiss isOutsideWorkDir := err != nil || strings.HasPrefix(relPath, "..") isSkillFile := isInSkillsPath(absFilePath, skillsPaths) + var permResult permission.PermissionResult // Request permission for files outside working directory, unless it's a skill file. if isOutsideWorkDir && !isSkillFile { sessionID := GetSessionFromContext(ctx) @@ -81,7 +82,7 @@ func NewViewTool(lspClients *csync.Map[string, *lsp.Client], permissions permiss return fantasy.ToolResponse{}, fmt.Errorf("session ID is required for accessing files outside working directory") } - granted, err := permissions.Request(ctx, + permResult, err = permissions.Request(ctx, permission.CreatePermissionRequest{ SessionID: sessionID, Path: absFilePath, @@ -95,7 +96,10 @@ func NewViewTool(lspClients *csync.Map[string, *lsp.Client], permissions permiss if err != nil { return fantasy.ToolResponse{}, err } - if !granted { + if !permResult.Granted { + if permResult.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(permResult.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } } @@ -192,7 +196,7 @@ func NewViewTool(lspClients *csync.Map[string, *lsp.Client], permissions permiss output += getDiagnostics(filePath, lspClients) filetracker.RecordRead(filePath) return fantasy.WithResponseMetadata( - fantasy.NewTextResponse(output), + fantasy.NewTextResponse(permResult.AppendCommentary(output)), ViewResponseMetadata{ FilePath: filePath, Content: content, diff --git a/internal/agent/tools/write.go b/internal/agent/tools/write.go index 8becaea3c08157897dcece7b3d5d4de5cb2ee929..aad20662f3644259eae820f0a9bcbd4bbdae2226 100644 --- a/internal/agent/tools/write.go +++ b/internal/agent/tools/write.go @@ -122,7 +122,10 @@ func NewWriteTool(lspClients *csync.Map[string, *lsp.Client], permissions permis if err != nil { return fantasy.ToolResponse{}, err } - if !p { + if !p.Granted { + if p.Message != "" { + return fantasy.NewTextErrorResponse("User denied permission." + permission.UserCommentaryTag(p.Message)), nil + } return fantasy.ToolResponse{}, permission.ErrorPermissionDenied } @@ -161,7 +164,7 @@ func NewWriteTool(lspClients *csync.Map[string, *lsp.Client], permissions permis result := fmt.Sprintf("File successfully written: %s", filePath) result = fmt.Sprintf("\n%s\n", result) result += getDiagnostics(filePath, lspClients) - return fantasy.WithResponseMetadata(fantasy.NewTextResponse(result), + return fantasy.WithResponseMetadata(fantasy.NewTextResponse(p.AppendCommentary(result)), WriteResponseMetadata{ Diff: diff, Additions: additions, diff --git a/internal/permission/permission.go b/internal/permission/permission.go index 2209e26ea924535598982c5158900b5a93dc0a21..144e8ce0628223a0e55912b7c122078c02de2cf5 100644 --- a/internal/permission/permission.go +++ b/internal/permission/permission.go @@ -15,6 +15,14 @@ import ( var ErrorPermissionDenied = errors.New("user denied permission") +// UserCommentaryTag formats user commentary for the LLM. +func UserCommentaryTag(message string) string { + if message == "" { + return "" + } + return "\n\nUser feedback: " + message +} + type CreatePermissionRequest struct { SessionID string `json:"session_id"` ToolCallID string `json:"tool_call_id"` @@ -29,6 +37,7 @@ type PermissionNotification struct { ToolCallID string `json:"tool_call_id"` Granted bool `json:"granted"` Denied bool `json:"denied"` + Message string `json:"message,omitempty"` // User commentary or instructions. } type PermissionRequest struct { @@ -44,16 +53,30 @@ type PermissionRequest struct { type Service interface { pubsub.Subscriber[PermissionRequest] - GrantPersistent(permission PermissionRequest) - Grant(permission PermissionRequest) - Deny(permission PermissionRequest) - Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) + GrantPersistent(permission PermissionRequest, message string) + Grant(permission PermissionRequest, message string) + Deny(permission PermissionRequest, message string) + Request(ctx context.Context, opts CreatePermissionRequest) (PermissionResult, error) AutoApproveSession(sessionID string) SetSkipRequests(skip bool) SkipRequests() bool SubscribeNotifications(ctx context.Context) <-chan pubsub.Event[PermissionNotification] } +// PermissionResult contains the result of a permission request. +type PermissionResult struct { + Granted bool + Message string // User's commentary or instructions. +} + +// AppendCommentary appends user commentary to content if present. +func (r PermissionResult) AppendCommentary(content string) string { + if r.Message == "" { + return content + } + return content + UserCommentaryTag(r.Message) +} + type permissionService struct { *pubsub.Broker[PermissionRequest] @@ -61,7 +84,7 @@ type permissionService struct { workingDir string sessionPermissions []PermissionRequest sessionPermissionsMu sync.RWMutex - pendingRequests *csync.Map[string, chan bool] + pendingRequests *csync.Map[string, chan PermissionResult] autoApproveSessions map[string]bool autoApproveSessionsMu sync.RWMutex skip bool @@ -73,14 +96,15 @@ type permissionService struct { activeRequestMu sync.Mutex } -func (s *permissionService) GrantPersistent(permission PermissionRequest) { +func (s *permissionService) GrantPersistent(permission PermissionRequest, message string) { s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ ToolCallID: permission.ToolCallID, Granted: true, + Message: message, }) respCh, ok := s.pendingRequests.Get(permission.ID) if ok { - respCh <- true + respCh <- PermissionResult{Granted: true, Message: message} } s.sessionPermissionsMu.Lock() @@ -94,14 +118,15 @@ func (s *permissionService) GrantPersistent(permission PermissionRequest) { s.activeRequestMu.Unlock() } -func (s *permissionService) Grant(permission PermissionRequest) { +func (s *permissionService) Grant(permission PermissionRequest, message string) { s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ ToolCallID: permission.ToolCallID, Granted: true, + Message: message, }) respCh, ok := s.pendingRequests.Get(permission.ID) if ok { - respCh <- true + respCh <- PermissionResult{Granted: true, Message: message} } s.activeRequestMu.Lock() @@ -111,15 +136,16 @@ func (s *permissionService) Grant(permission PermissionRequest) { s.activeRequestMu.Unlock() } -func (s *permissionService) Deny(permission PermissionRequest) { +func (s *permissionService) Deny(permission PermissionRequest, message string) { s.notificationBroker.Publish(pubsub.CreatedEvent, PermissionNotification{ ToolCallID: permission.ToolCallID, Granted: false, Denied: true, + Message: message, }) respCh, ok := s.pendingRequests.Get(permission.ID) if ok { - respCh <- false + respCh <- PermissionResult{Granted: false, Message: message} } s.activeRequestMu.Lock() @@ -129,9 +155,9 @@ func (s *permissionService) Deny(permission PermissionRequest) { s.activeRequestMu.Unlock() } -func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) { +func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (PermissionResult, error) { if s.skip { - return true, nil + return PermissionResult{Granted: true}, nil } // tell the UI that a permission was requested @@ -144,7 +170,7 @@ func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRe // Check if the tool/action combination is in the allowlist commandKey := opts.ToolName + ":" + opts.Action if slices.Contains(s.allowedTools, commandKey) || slices.Contains(s.allowedTools, opts.ToolName) { - return true, nil + return PermissionResult{Granted: true}, nil } s.autoApproveSessionsMu.RLock() @@ -152,7 +178,7 @@ func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRe s.autoApproveSessionsMu.RUnlock() if autoApprove { - return true, nil + return PermissionResult{Granted: true}, nil } fileInfo, err := os.Stat(opts.Path) @@ -183,7 +209,7 @@ func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRe for _, p := range s.sessionPermissions { if p.ToolName == permission.ToolName && p.Action == permission.Action && p.SessionID == permission.SessionID && p.Path == permission.Path { s.sessionPermissionsMu.RUnlock() - return true, nil + return PermissionResult{Granted: true}, nil } } s.sessionPermissionsMu.RUnlock() @@ -192,7 +218,7 @@ func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRe s.activeRequest = &permission s.activeRequestMu.Unlock() - respCh := make(chan bool, 1) + respCh := make(chan PermissionResult, 1) s.pendingRequests.Set(permission.ID, respCh) defer s.pendingRequests.Del(permission.ID) @@ -201,9 +227,9 @@ func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRe select { case <-ctx.Done(): - return false, ctx.Err() - case granted := <-respCh: - return granted, nil + return PermissionResult{Granted: false}, ctx.Err() + case result := <-respCh: + return result, nil } } @@ -234,6 +260,6 @@ func NewPermissionService(workingDir string, skip bool, allowedTools []string) S autoApproveSessions: make(map[string]bool), skip: skip, allowedTools: allowedTools, - pendingRequests: csync.NewMap[string, chan bool](), + pendingRequests: csync.NewMap[string, chan PermissionResult](), } } diff --git a/internal/permission/permission_test.go b/internal/permission/permission_test.go index 79930f3ae1e2ef15257f09724fef64d3ea28dada..f4cc27b6e60a652a0386a61ae58b2aa31c737ef4 100644 --- a/internal/permission/permission_test.go +++ b/internal/permission/permission_test.go @@ -92,7 +92,7 @@ func TestPermissionService_SkipMode(t *testing.T) { if err != nil { t.Errorf("unexpected error: %v", err) } - if !result { + if !result.Granted { t.Error("expected permission to be granted in skip mode") } } @@ -110,7 +110,7 @@ func TestPermissionService_SequentialProperties(t *testing.T) { Path: "/tmp/test.txt", } - var result1 bool + var result1 PermissionResult var wg sync.WaitGroup wg.Add(1) @@ -125,10 +125,10 @@ func TestPermissionService_SequentialProperties(t *testing.T) { event := <-events permissionReq = event.Payload - service.GrantPersistent(permissionReq) + service.GrantPersistent(permissionReq, "") wg.Wait() - assert.True(t, result1, "First request should be granted") + assert.True(t, result1.Granted, "First request should be granted") // Second identical request should be automatically approved due to persistent permission req2 := CreatePermissionRequest{ @@ -141,7 +141,7 @@ func TestPermissionService_SequentialProperties(t *testing.T) { } result2, err := service.Request(t.Context(), req2) require.NoError(t, err) - assert.True(t, result2, "Second request should be auto-approved") + assert.True(t, result2.Granted, "Second request should be auto-approved") }) t.Run("Sequential requests with temporary grants", func(t *testing.T) { service := NewPermissionService("/tmp", false, []string{}) @@ -156,7 +156,7 @@ func TestPermissionService_SequentialProperties(t *testing.T) { } events := service.Subscribe(t.Context()) - var result1 bool + var result1 PermissionResult var wg sync.WaitGroup wg.Go(func() { @@ -167,11 +167,11 @@ func TestPermissionService_SequentialProperties(t *testing.T) { event := <-events permissionReq = event.Payload - service.Grant(permissionReq) + service.Grant(permissionReq, "") wg.Wait() - assert.True(t, result1, "First request should be granted") + assert.True(t, result1.Granted, "First request should be granted") - var result2 bool + var result2 PermissionResult wg.Go(func() { result2, _ = service.Request(t.Context(), req) @@ -179,9 +179,9 @@ func TestPermissionService_SequentialProperties(t *testing.T) { event = <-events permissionReq = event.Payload - service.Deny(permissionReq) + service.Deny(permissionReq, "") wg.Wait() - assert.False(t, result2, "Second request should be denied") + assert.False(t, result2.Granted, "Second request should be denied") }) t.Run("Concurrent requests with different outcomes", func(t *testing.T) { service := NewPermissionService("/tmp", false, []string{}) @@ -189,7 +189,7 @@ func TestPermissionService_SequentialProperties(t *testing.T) { events := service.Subscribe(t.Context()) var wg sync.WaitGroup - results := make([]bool, 3) + results := make([]PermissionResult, 3) requests := []CreatePermissionRequest{ { @@ -228,17 +228,17 @@ func TestPermissionService_SequentialProperties(t *testing.T) { event := <-events switch event.Payload.ToolName { case "tool1": - service.Grant(event.Payload) + service.Grant(event.Payload, "") case "tool2": - service.GrantPersistent(event.Payload) + service.GrantPersistent(event.Payload, "") case "tool3": - service.Deny(event.Payload) + service.Deny(event.Payload, "") } } wg.Wait() grantedCount := 0 for _, result := range results { - if result { + if result.Granted { grantedCount++ } } @@ -248,6 +248,6 @@ func TestPermissionService_SequentialProperties(t *testing.T) { secondReq.Description = "Repeat of second request" result, err := service.Request(t.Context(), secondReq) require.NoError(t, err) - assert.True(t, result, "Repeated request should be auto-approved due to persistent permission") + assert.True(t, result.Granted, "Repeated request should be auto-approved due to persistent permission") }) } diff --git a/internal/tui/tui.go b/internal/tui/tui.go index 7586cd8a494541e52a3e403fe0374b772bd7d332..494649fc798557187e897c2649422899306c59cc 100644 --- a/internal/tui/tui.go +++ b/internal/tui/tui.go @@ -325,11 +325,11 @@ func (a *appModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { case permissions.PermissionResponseMsg: switch msg.Action { case permissions.PermissionAllow: - a.app.Permissions.Grant(msg.Permission) + a.app.Permissions.Grant(msg.Permission, "") case permissions.PermissionAllowForSession: - a.app.Permissions.GrantPersistent(msg.Permission) + a.app.Permissions.GrantPersistent(msg.Permission, "") case permissions.PermissionDeny: - a.app.Permissions.Deny(msg.Permission) + a.app.Permissions.Deny(msg.Permission, "") } return a, nil case splash.OnboardingCompleteMsg: diff --git a/internal/ui/dialog/actions.go b/internal/ui/dialog/actions.go index b5db01692437dbee4b11b77da47b68f258b090e9..ffc603774353c73a32f8261fb928d04d56ee9a44 100644 --- a/internal/ui/dialog/actions.go +++ b/internal/ui/dialog/actions.go @@ -61,6 +61,7 @@ type ( ActionPermissionResponse struct { Permission permission.PermissionRequest Action PermissionAction + Commentary string // User's commentary or instructions for the action. } // ActionRunCustomCommand is a message to run a custom command. ActionRunCustomCommand struct { diff --git a/internal/ui/dialog/permissions.go b/internal/ui/dialog/permissions.go index 8f2ca1ed27e7eff5096bcb33c8f516a07fe2dd88..5a1155758c1ce2b919560b1d520aba92901993cb 100644 --- a/internal/ui/dialog/permissions.go +++ b/internal/ui/dialog/permissions.go @@ -7,6 +7,7 @@ import ( "charm.land/bubbles/v2/help" "charm.land/bubbles/v2/key" + "charm.land/bubbles/v2/textinput" "charm.land/bubbles/v2/viewport" tea "charm.land/bubbletea/v2" "charm.land/lipgloss/v2" @@ -73,6 +74,10 @@ type Permissions struct { unifiedDiffContent string splitDiffContent string + // Commentary input for user feedback. + input textinput.Model + inputFocused bool + help help.Model keyMap permissionsKeyMap } @@ -85,6 +90,9 @@ type permissionsKeyMap struct { Allow key.Binding AllowSession key.Binding Deny key.Binding + CtrlAllow key.Binding + CtrlAllowSession key.Binding + CtrlDeny key.Binding Close key.Binding ToggleDiffMode key.Binding ToggleFullscreen key.Binding @@ -94,6 +102,7 @@ type permissionsKeyMap struct { ScrollRight key.Binding Choose key.Binding Scroll key.Binding + FocusInput key.Binding } func defaultPermissionsKeyMap() permissionsKeyMap { @@ -115,17 +124,29 @@ func defaultPermissionsKeyMap() permissionsKeyMap { key.WithHelp("enter", "confirm"), ), Allow: key.NewBinding( - key.WithKeys("a", "A", "ctrl+a"), + key.WithKeys("a"), key.WithHelp("a", "allow"), ), AllowSession: key.NewBinding( - key.WithKeys("s", "S", "ctrl+s"), + key.WithKeys("s"), key.WithHelp("s", "allow session"), ), Deny: key.NewBinding( - key.WithKeys("d", "D"), + key.WithKeys("d"), key.WithHelp("d", "deny"), ), + CtrlAllow: key.NewBinding( + key.WithKeys("ctrl+a"), + key.WithHelp("ctrl+a", "allow"), + ), + CtrlAllowSession: key.NewBinding( + key.WithKeys("ctrl+s"), + key.WithHelp("ctrl+s", "session"), + ), + CtrlDeny: key.NewBinding( + key.WithKeys("ctrl+d"), + key.WithHelp("ctrl+d", "deny"), + ), Close: CloseKey, ToggleDiffMode: key.NewBinding( key.WithKeys("t"), @@ -159,6 +180,10 @@ func defaultPermissionsKeyMap() permissionsKeyMap { key.WithKeys("shift+left", "shift+down", "shift+up", "shift+right"), key.WithHelp("shift+←↓↑→", "scroll"), ), + FocusInput: key.NewBinding( + key.WithKeys("/"), + key.WithHelp("/", "add comment"), + ), } } @@ -195,11 +220,18 @@ func NewPermissions(com *common.Common, perm permission.PermissionRequest, opts HalfPageDown: key.NewBinding(key.WithDisabled()), } + // Configure text input for user commentary. + input := textinput.New() + input.SetVirtualCursor(false) + input.Placeholder = "Feedback for the agent (optional)..." + input.SetStyles(com.Styles.TextInput) + p := &Permissions{ com: com, permission: perm, selectedOption: 0, viewport: vp, + input: input, help: h, keyMap: km, } @@ -227,10 +259,42 @@ func (*Permissions) ID() string { func (p *Permissions) HandleMsg(msg tea.Msg) Action { switch msg := msg.(type) { case tea.KeyPressMsg: + // When input is focused, handle navigation and shortcuts first. + if p.inputFocused { + switch { + case key.Matches(msg, p.keyMap.Close): + // Escape unfocuses the input. + p.inputFocused = false + p.input.Blur() + return nil + case key.Matches(msg, p.keyMap.Select): + // Enter confirms the current selection with the comment. + return p.selectCurrentOption() + case key.Matches(msg, p.keyMap.Tab): + p.selectedOption = (p.selectedOption + 1) % 3 + return nil + case key.Matches(msg, p.keyMap.CtrlAllow): + return p.respond(PermissionAllow) + case key.Matches(msg, p.keyMap.CtrlAllowSession): + return p.respond(PermissionAllowForSession) + case key.Matches(msg, p.keyMap.CtrlDeny): + return p.respond(PermissionDeny) + default: + // Pass other keys to the text input. + p.input, _ = p.input.Update(msg) + return nil + } + } + + // Normal dialog navigation when input is not focused. switch { case key.Matches(msg, p.keyMap.Close): // Escape denies the permission request. return p.respond(PermissionDeny) + case key.Matches(msg, p.keyMap.FocusInput): + p.inputFocused = true + p.input.Focus() + return nil case key.Matches(msg, p.keyMap.Right), key.Matches(msg, p.keyMap.Tab): p.selectedOption = (p.selectedOption + 1) % 3 case key.Matches(msg, p.keyMap.Left): @@ -238,11 +302,11 @@ func (p *Permissions) HandleMsg(msg tea.Msg) Action { p.selectedOption = (p.selectedOption + 2) % 3 case key.Matches(msg, p.keyMap.Select): return p.selectCurrentOption() - case key.Matches(msg, p.keyMap.Allow): + case key.Matches(msg, p.keyMap.Allow), key.Matches(msg, p.keyMap.CtrlAllow): return p.respond(PermissionAllow) - case key.Matches(msg, p.keyMap.AllowSession): + case key.Matches(msg, p.keyMap.AllowSession), key.Matches(msg, p.keyMap.CtrlAllowSession): return p.respond(PermissionAllowForSession) - case key.Matches(msg, p.keyMap.Deny): + case key.Matches(msg, p.keyMap.Deny), key.Matches(msg, p.keyMap.CtrlDeny): return p.respond(PermissionDeny) case key.Matches(msg, p.keyMap.ToggleDiffMode): if p.hasDiffView() { @@ -291,6 +355,7 @@ func (p *Permissions) respond(action PermissionAction) tea.Msg { return ActionPermissionResponse{ Permission: p.permission, Action: action, + Commentary: p.input.Value(), } } @@ -336,13 +401,20 @@ func (p *Permissions) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor { contentWidth := p.calculateContentWidth(width) header := p.renderHeader(contentWidth) buttons := p.renderButtons(contentWidth) + + // Render the input field. + p.input.SetWidth(contentWidth - t.Dialog.InputPrompt.GetHorizontalFrameSize() - 1) + inputView := t.Dialog.InputPrompt.Render(p.input.View()) + helpView := p.help.View(p) // Calculate available height for content. headerHeight := lipgloss.Height(header) buttonsHeight := lipgloss.Height(buttons) + inputHeight := lipgloss.Height(inputView) helpHeight := lipgloss.Height(helpView) - frameHeight := dialogStyle.GetVerticalFrameSize() + layoutSpacingLines + // Add extra spacing lines for input section. + frameHeight := dialogStyle.GetVerticalFrameSize() + layoutSpacingLines + 2 p.defaultDiffSplitMode = width >= splitModeMinWidth @@ -361,7 +433,7 @@ func (p *Permissions) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor { availableHeight = maxHeight - fixedHeight } } else { - availableHeight = maxHeight - headerHeight - buttonsHeight - helpHeight - frameHeight + availableHeight = maxHeight - headerHeight - buttonsHeight - inputHeight - helpHeight - frameHeight } // Determine if scrollbar is needed. @@ -379,6 +451,7 @@ func (p *Permissions) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor { var content string var scrollbar string + availableHeight = min(availableHeight, lipgloss.Height(renderedContent)) p.viewport.SetWidth(viewportWidth) p.viewport.SetHeight(availableHeight) if p.viewportDirty { @@ -387,6 +460,7 @@ func (p *Permissions) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor { p.viewportDirty = false } content = p.viewport.View() + if needsScrollbar { scrollbar = common.Scrollbar(t, availableHeight, p.viewport.TotalLineCount(), availableHeight, p.viewport.YOffset()) } @@ -400,11 +474,28 @@ func (p *Permissions) Draw(scr uv.Screen, area uv.Rectangle) *tea.Cursor { if content != "" { parts = append(parts, "", content) } - parts = append(parts, "", buttons, "", helpView) + parts = append(parts, "", inputView, "", buttons, "", helpView) innerContent := lipgloss.JoinVertical(lipgloss.Left, parts...) - DrawCenterCursor(scr, area, dialogStyle.Render(innerContent), nil) - return nil + + var cur *tea.Cursor + if p.inputFocused { + cur = p.input.Cursor() + if cur != nil { + // Calculate Y offset: header + empty line + content + empty line. + yOffset := headerHeight + if content != "" { + yOffset += 1 + lipgloss.Height(content) + } + yOffset += 1 // Empty line before input. + + // Add dialog frame offsets. + cur.X += dialogStyle.GetHorizontalFrameSize()/2 + 1 + cur.Y += dialogStyle.GetVerticalFrameSize()/2 + yOffset + 1 + } + } + DrawCenterCursor(scr, area, dialogStyle.Render(innerContent), cur) + return cur } func (p *Permissions) renderHeader(contentWidth int) string { @@ -734,9 +825,22 @@ func (p *Permissions) canScroll() bool { // ShortHelp implements [help.KeyMap]. func (p *Permissions) ShortHelp() []key.Binding { + // When input is focused, show different help. + if p.inputFocused { + return []key.Binding{ + p.keyMap.Tab, + p.keyMap.Select, + p.keyMap.CtrlAllow, + p.keyMap.CtrlAllowSession, + p.keyMap.CtrlDeny, + p.keyMap.Close, + } + } + bindings := []key.Binding{ p.keyMap.Choose, p.keyMap.Select, + p.keyMap.FocusInput, p.keyMap.Close, } diff --git a/internal/ui/model/ui.go b/internal/ui/model/ui.go index 0f59e10bee7a6c7ed3a12ef2edae8cf74f5de5d6..69f7a580dc4f5df02d11adab88e9780a5e9f57b8 100644 --- a/internal/ui/model/ui.go +++ b/internal/ui/model/ui.go @@ -1190,11 +1190,11 @@ func (m *UI) handleDialogMsg(msg tea.Msg) tea.Cmd { m.dialog.CloseDialog(dialog.PermissionsID) switch msg.Action { case dialog.PermissionAllow: - m.com.App.Permissions.Grant(msg.Permission) + m.com.App.Permissions.Grant(msg.Permission, msg.Commentary) case dialog.PermissionAllowForSession: - m.com.App.Permissions.GrantPersistent(msg.Permission) + m.com.App.Permissions.GrantPersistent(msg.Permission, msg.Commentary) case dialog.PermissionDeny: - m.com.App.Permissions.Deny(msg.Permission) + m.com.App.Permissions.Deny(msg.Permission, msg.Commentary) } case dialog.ActionFilePickerSelected: