diff --git a/cmd/mcp-http/main.go b/cmd/mcp-http/main.go index 68a67b99..f025a3aa 100644 --- a/cmd/mcp-http/main.go +++ b/cmd/mcp-http/main.go @@ -24,6 +24,7 @@ import ( "github.com/teamwork/mcp/internal/auth" "github.com/teamwork/mcp/internal/cli" "github.com/teamwork/mcp/internal/config" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/request" "github.com/teamwork/mcp/internal/toolsets" "github.com/teamwork/mcp/internal/twchat" @@ -321,7 +322,7 @@ func logMiddleware(logger *slog.Logger, next http.Handler) http.Handler { slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", logsafe.String(string(reqBody))), slog.Int("response_status", rw.StatusCode()), slog.Any("response_headers", rw.Header()), slog.String("response_body", string(rw.Body())), @@ -388,7 +389,7 @@ func sseLogMiddleware(logger *slog.Logger, next http.Handler) http.Handler { slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", logsafe.String(string(reqBody))), slog.Int("response_status", rw.StatusCode()), slog.Any("response_headers", rw.Header()), slog.String("response_body", string(rw.Body())), diff --git a/cmd/mcptest/.env.example b/cmd/mcptest/.env.example new file mode 100644 index 00000000..df749e80 --- /dev/null +++ b/cmd/mcptest/.env.example @@ -0,0 +1,10 @@ +# Copy this file to .env (or copy ..\sdktest\.env here) and fill in. +# .env is gitignored. + +TWAPI_SERVER=https://yoursite.teamwork.com +TWAPI_TOKEN=your-token-here +PROJECT_ID=12345 + +# Optional: override auth detection. Defaults to basic if token starts +# with "twp_", bearer otherwise. +# TWAPI_AUTH=basic diff --git a/cmd/mcptest/.gitignore b/cmd/mcptest/.gitignore new file mode 100644 index 00000000..4c49bd78 --- /dev/null +++ b/cmd/mcptest/.gitignore @@ -0,0 +1 @@ +.env diff --git a/cmd/mcptest/main.go b/cmd/mcptest/main.go new file mode 100644 index 00000000..dc773e97 --- /dev/null +++ b/cmd/mcptest/main.go @@ -0,0 +1,575 @@ +// mcptest walks through the Custom Items MCP tool handlers against a real +// Teamwork.com site. It bypasses the MCP server transport and invokes each +// tool's Handler directly with the same JSON payload an LLM would send, so +// you can see end-to-end behaviour — including field-name resolution, value +// coercion, twId↔name translation and the schema cache — without standing +// up an MCP server or LLM client. +// +// Configuration is read from .env (or the path given by -config). The auth +// mode (basic vs bearer) is auto-detected from the token prefix; override +// with TWAPI_AUTH. Same .env shape as sdktest. +// +// Usage: +// +// cd c:\programming\mcptest +// copy ..\sdktest\.env .env # if you don't already have one here +// go run . +package main + +import ( + "bufio" + "context" + "encoding/json" + "errors" + "flag" + "fmt" + "os" + "strconv" + "strings" + + "github.com/modelcontextprotocol/go-sdk/mcp" + + "github.com/teamwork/mcp/internal/toolsets" + "github.com/teamwork/mcp/internal/twprojects" + twapi "github.com/teamwork/twapi-go-sdk" + "github.com/teamwork/twapi-go-sdk/session" +) + +func main() { + configPath := preScanConfig(os.Args[1:], ".env") + if err := loadEnvFile(configPath); err != nil { + fmt.Fprintf(os.Stderr, "failed to read %s: %v\n", configPath, err) + os.Exit(2) + } + + flag.StringVar(&configPath, "config", configPath, "Path to .env file (ignored if missing)") + server := flag.String("server", os.Getenv("TWAPI_SERVER"), "Teamwork base URL (env TWAPI_SERVER)") + token := flag.String("token", os.Getenv("TWAPI_TOKEN"), "Bearer token / API key (env TWAPI_TOKEN)") + projectID := flag.Int64("project", envInt64("PROJECT_ID"), "Project ID to test on (env PROJECT_ID)") + keep := flag.Bool("keep", false, "Don't delete created artefacts so you can inspect them in the UI") + step := flag.Bool("step", false, "Pause for ENTER between steps") + flag.Parse() + + if *server == "" || *token == "" || *projectID == 0 { + fmt.Fprintln(os.Stderr, "server, token and project are all required (set in .env, env vars, or flags)") + flag.Usage() + os.Exit(2) + } + *server = strings.TrimSuffix(*server, "/") + + authMode := strings.ToLower(strings.TrimSpace(os.Getenv("TWAPI_AUTH"))) + if authMode == "" { + if strings.HasPrefix(*token, "twp_") { + authMode = "basic" + } else { + authMode = "bearer" + } + } + var sess twapi.Session + switch authMode { + case "basic": + fmt.Println("auth: HTTP Basic (personal API key)") + sess = session.NewBasicAuth(*token, "x", *server) + case "bearer": + fmt.Println("auth: Bearer token") + sess = session.NewBearerToken(*token, *server) + default: + fmt.Fprintf(os.Stderr, "unknown TWAPI_AUTH %q (want basic|bearer)\n", authMode) + os.Exit(2) + } + engine := twapi.NewEngine(sess) + + r := &runner{ + engine: engine, + projectID: *projectID, + keep: *keep, + step: *step, + stdin: bufio.NewReader(os.Stdin), + ctx: context.Background(), + } + if err := r.run(); err != nil { + fmt.Fprintf(os.Stderr, "FAILED: %v\n", err) + os.Exit(1) + } + fmt.Println("DONE.") +} + +type runner struct { + engine *twapi.Engine + projectID int64 + keep bool + step bool + stdin *bufio.Reader + ctx context.Context + + customItemID int64 + statusID int64 + notesID int64 + notesTwID string + recordID int64 + recordIDs []int64 +} + +func (r *runner) run() error { + defer r.cleanup() + + steps := []struct { + label string + fn func() error + }{ + {"LIST existing custom items via MCP", r.stepListCustomItems}, + {"CREATE custom item via MCP", r.stepCreateCustomItem}, + {`CREATE field "Notes" (text-short) via MCP`, r.stepCreateNotesField}, + {`CREATE field "Status" (dropdown) via MCP`, r.stepCreateStatusField}, + {"LIST fields via MCP — verify twIds", r.stepListFields}, + {"CREATE record by FIELD NAME (Notes + Status)", r.stepCreateRecordByName}, + {"GET record via MCP — verify twId→name translation", r.stepGetRecord}, + {"UPDATE record by field name (clear section, change Status by label)", r.stepUpdateRecord}, + {"LIST records via MCP — verify translation across a page", r.stepListRecords}, + {"CREATE 2 extra records for bulk delete", r.stepCreateExtraRecords}, + {"NEGATIVE: unknown field name should error clearly", r.stepNegativeUnknownField}, + } + for _, s := range steps { + fmt.Printf("\n=== %s ===\n", s.label) + if r.step { + fmt.Print("press ENTER to continue: ") + _, _ = r.stdin.ReadString('\n') + } + if err := s.fn(); err != nil { + return err + } + } + return nil +} + +// --------------------------------------------------------------------------- +// MCP invocation helpers +// --------------------------------------------------------------------------- + +// callTool marshals args to JSON and invokes the tool's handler directly. +// Returns the structured text result and any error from the handler, plus a +// bool indicating whether the result was an error result (IsError). +func (r *runner) callTool(tool toolsets.ToolWrapper, args map[string]any) (text string, isError bool, err error) { + raw, err := json.Marshal(args) + if err != nil { + return "", false, fmt.Errorf("marshal args: %w", err) + } + request := &mcp.CallToolRequest{ + Params: &mcp.CallToolParamsRaw{ + Name: tool.Tool.Name, + Arguments: json.RawMessage(raw), + }, + } + result, err := tool.Handler(r.ctx, request) + if err != nil { + return "", false, err + } + if result == nil { + return "", false, errors.New("nil result") + } + for _, content := range result.Content { + if text, ok := content.(*mcp.TextContent); ok { + return text.Text, result.IsError, nil + } + } + return "", result.IsError, errors.New("no text content in result") +} + +// callToolExpectOK calls a tool, prints the response, and returns an error +// if the handler returned err or an IsError result. +func (r *runner) callToolExpectOK(label string, tool toolsets.ToolWrapper, args map[string]any) (string, error) { + text, isError, err := r.callTool(tool, args) + if err != nil { + return "", fmt.Errorf("%s: %w", label, err) + } + if isError { + return "", fmt.Errorf("%s returned error result: %s", label, text) + } + prettyPrint(text) + return text, nil +} + +func prettyPrint(text string) { + // If the body is JSON, pretty-print it; otherwise emit raw. + var anyVal any + if err := json.Unmarshal([]byte(text), &anyVal); err == nil { + formatted, _ := json.MarshalIndent(anyVal, " ", " ") + fmt.Printf(" %s\n", string(formatted)) + return + } + fmt.Printf(" %s\n", text) +} + +// --------------------------------------------------------------------------- +// Steps +// --------------------------------------------------------------------------- + +func (r *runner) stepListCustomItems() error { + _, err := r.callToolExpectOK("list_custom_items", twprojects.CustomItemList(r.engine), map[string]any{ + "project_id": r.projectID, + }) + return err +} + +func (r *runner) stepCreateCustomItem() error { + name := fmt.Sprintf("MCPTest-%d", r.projectID) + text, err := r.callToolExpectOK("create_custom_item", twprojects.CustomItemCreate(r.engine), map[string]any{ + "project_id": r.projectID, + "display_name": name, + "label_singular": "MCP Record", + "label_plural": "MCP Records", + }) + if err != nil { + return err + } + r.customItemID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract custom item id: %w", err) + } + fmt.Printf(" → captured customItemID=%d\n", r.customItemID) + return nil +} + +func (r *runner) stepCreateNotesField() error { + text, err := r.callToolExpectOK("create_custom_item_field", twprojects.CustomItemFieldCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "display_name": "Notes", + "type": "text-short", + }) + if err != nil { + return err + } + r.notesID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract field id: %w", err) + } + fmt.Printf(" → captured notesFieldID=%d\n", r.notesID) + return nil +} + +func (r *runner) stepCreateStatusField() error { + text, err := r.callToolExpectOK("create_custom_item_field", twprojects.CustomItemFieldCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "display_name": "Status", + "type": "dropdown", + "tw_type": "status", + "options": []map[string]any{ + {"label": "Active", "color": "#22c55e"}, + {"label": "Pending", "color": "#facc15"}, + {"label": "Closed", "color": "#94a3b8"}, + }, + }) + if err != nil { + return err + } + r.statusID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract field id: %w", err) + } + fmt.Printf(" → captured statusFieldID=%d\n", r.statusID) + return nil +} + +func (r *runner) stepListFields() error { + text, err := r.callToolExpectOK("list_custom_item_fields", + twprojects.CustomItemFieldList(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + }) + if err != nil { + return err + } + // Pull the Notes field twId out so we can compare against the value + // coming back in the record step. + var listResp struct { + CustomItemFields []struct { + ID int64 `json:"id"` + TwID string `json:"twId"` + DisplayName string `json:"displayName"` + } `json:"customItemFields"` + } + if err := json.Unmarshal([]byte(text), &listResp); err == nil { + for _, field := range listResp.CustomItemFields { + if field.DisplayName == "Notes" { + r.notesTwID = field.TwID + fmt.Printf(" → captured notesTwID=%q\n", r.notesTwID) + } + } + } + return nil +} + +func (r *runner) stepCreateRecordByName() error { + text, err := r.callToolExpectOK("create_custom_item_record", + twprojects.CustomItemRecordCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "name": "Acme Inc", + "field_values": []map[string]any{ + {"field_name": "Notes", "value": "initial contact"}, + // Pass the option by LABEL — the handler should resolve it + // to the option twId before sending. + {"field_name": "Status", "value": "Active"}, + }, + }) + if err != nil { + return err + } + r.recordID, err = extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract record id: %w", err) + } + r.recordIDs = append(r.recordIDs, r.recordID) + fmt.Printf(" → captured recordID=%d\n", r.recordID) + return nil +} + +func (r *runner) stepGetRecord() error { + text, err := r.callToolExpectOK("get_custom_item_record", + twprojects.CustomItemRecordGet(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": r.recordID, + }) + if err != nil { + return err + } + // Sanity: confirm field values came back keyed by name, not twId. + if strings.Contains(text, `"Notes"`) { + fmt.Println(" ✓ field values keyed by name (Notes)") + } else { + fmt.Println(" ! WARNING: Notes not present in response by display name") + } + if strings.Contains(text, `"Active"`) { + fmt.Println(" ✓ Status value translated back to label (Active)") + } else { + fmt.Println(" ! WARNING: Status label not present in response") + } + if r.notesTwID != "" && strings.Contains(text, r.notesTwID) { + fmt.Printf(" ! WARNING: raw twId %q leaked into response\n", r.notesTwID) + } + return nil +} + +func (r *runner) stepUpdateRecord() error { + _, err := r.callToolExpectOK("update_custom_item_record", + twprojects.CustomItemRecordUpdate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": r.recordID, + "name": "Acme Inc (updated via MCP)", + "clear_section": true, + "field_values": []map[string]any{ + {"field_name": "Status", "value": "Pending"}, // by label + {"field_name": "Notes", "value": "follow-up next week"}, + }, + }) + return err +} + +func (r *runner) stepListRecords() error { + text, err := r.callToolExpectOK("list_custom_item_records", + twprojects.CustomItemRecordList(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + }) + if err != nil { + return err + } + if strings.Contains(text, `"Notes"`) && strings.Contains(text, `"Status"`) { + fmt.Println(" ✓ list response uses display names for field keys") + } + return nil +} + +func (r *runner) stepCreateExtraRecords() error { + for i := 0; i < 2; i++ { + text, err := r.callToolExpectOK("create_custom_item_record (extra)", + twprojects.CustomItemRecordCreate(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "name": fmt.Sprintf("bulk-target-%d", i+1), + }) + if err != nil { + return err + } + id, err := extractTrailingID(text) + if err != nil { + return fmt.Errorf("extract extra record id: %w", err) + } + r.recordIDs = append(r.recordIDs, id) + } + return nil +} + +func (r *runner) stepNegativeUnknownField() error { + tool := twprojects.CustomItemRecordCreate(r.engine) + text, isError, err := r.callTool(tool, map[string]any{ + "custom_item_id": r.customItemID, + "name": "negative-test", + "field_values": []map[string]any{ + {"field_name": "NoSuchField", "value": "x"}, + }, + }) + if err != nil { + return fmt.Errorf("negative test: %w", err) + } + if !isError { + return fmt.Errorf("expected an error result for unknown field, got success: %s", text) + } + fmt.Printf(" ✓ unknown-field error surfaced: %s\n", strings.TrimSpace(text)) + return nil +} + +// --------------------------------------------------------------------------- +// Cleanup +// --------------------------------------------------------------------------- + +func (r *runner) cleanup() { + if r.keep { + fmt.Println("\n--- KEEP set, leaving artefacts in place ---") + if r.customItemID != 0 { + fmt.Printf(" customItemID = %d\n", r.customItemID) + } + if r.notesID != 0 { + fmt.Printf(" notesFieldID = %d\n", r.notesID) + } + if r.statusID != 0 { + fmt.Printf(" statusFieldID = %d\n", r.statusID) + } + if len(r.recordIDs) > 0 { + fmt.Printf(" recordIDs = %v\n", r.recordIDs) + } + return + } + + fmt.Println("\n--- cleanup ---") + + if len(r.recordIDs) > 0 && r.customItemID != 0 { + fmt.Printf(" bulk-delete %d records via MCP\n", len(r.recordIDs)) + _, isError, err := r.callTool(twprojects.CustomItemRecordBulkDelete(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "ids": asAnyInts(r.recordIDs), + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! bulk delete records failed: err=%v\n", err) + } + } + + for _, fieldID := range []int64{r.notesID, r.statusID} { + if fieldID == 0 || r.customItemID == 0 { + continue + } + fmt.Printf(" delete field %d via MCP\n", fieldID) + _, isError, err := r.callTool(twprojects.CustomItemFieldDelete(r.engine), map[string]any{ + "custom_item_id": r.customItemID, + "id": fieldID, + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! delete field %d failed: err=%v\n", fieldID, err) + } + } + + if r.customItemID != 0 { + fmt.Printf(" delete custom item %d via MCP\n", r.customItemID) + _, isError, err := r.callTool(twprojects.CustomItemDelete(r.engine), map[string]any{ + "id": r.customItemID, + }) + if err != nil || isError { + fmt.Fprintf(os.Stderr, " ! delete custom item failed: err=%v\n", err) + } + } +} + +// asAnyInts converts []int64 to []any (what JSON-encoded numeric arrays look +// like after a typical map[string]any unmarshal). +func asAnyInts(ids []int64) []any { + out := make([]any, len(ids)) + for i, id := range ids { + out[i] = id + } + return out +} + +// extractTrailingID pulls the last whitespace-separated token from the text, +// assuming a result like "Custom item created successfully with ID 1234". +func extractTrailingID(text string) (int64, error) { + trimmed := strings.TrimSpace(text) + if trimmed == "" { + return 0, errors.New("empty text") + } + parts := strings.Fields(trimmed) + last := parts[len(parts)-1] + id, err := strconv.ParseInt(last, 10, 64) + if err != nil { + return 0, fmt.Errorf("trailing token %q is not an integer: %w", last, err) + } + return id, nil +} + +// --------------------------------------------------------------------------- +// .env loader (same shape as sdktest) +// --------------------------------------------------------------------------- + +func preScanConfig(args []string, fallback string) string { + for i := 0; i < len(args); i++ { + arg := args[i] + switch { + case arg == "-config" || arg == "--config": + if i+1 < len(args) { + return args[i+1] + } + case strings.HasPrefix(arg, "-config=") || strings.HasPrefix(arg, "--config="): + return arg[strings.IndexByte(arg, '=')+1:] + case arg == "--": + return fallback + } + } + return fallback +} + +func loadEnvFile(path string) error { + f, err := os.Open(path) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + lineNo := 0 + for scanner.Scan() { + lineNo++ + line := strings.TrimSpace(scanner.Text()) + if line == "" || strings.HasPrefix(line, "#") { + continue + } + eq := strings.IndexByte(line, '=') + if eq <= 0 { + return fmt.Errorf("%s line %d: expected KEY=VALUE", path, lineNo) + } + key := strings.TrimSpace(line[:eq]) + value := strings.TrimSpace(line[eq+1:]) + if n := len(value); n >= 2 { + first, last := value[0], value[n-1] + if (first == '"' && last == '"') || (first == '\'' && last == '\'') { + value = value[1 : n-1] + } + } + if _, set := os.LookupEnv(key); set { + continue + } + if err := os.Setenv(key, value); err != nil { + return fmt.Errorf("%s line %d: setenv %s: %w", path, lineNo, key, err) + } + } + return scanner.Err() +} + +func envInt64(key string) int64 { + value, ok := os.LookupEnv(key) + if !ok || value == "" { + return 0 + } + parsed, err := strconv.ParseInt(value, 10, 64) + if err != nil { + return 0 + } + return parsed +} diff --git a/docs/tool-reference.md b/docs/tool-reference.md index dbaadac4..ca3c589b 100644 --- a/docs/tool-reference.md +++ b/docs/tool-reference.md @@ -53,6 +53,7 @@ Project, category, template, member, custom field, and custom item (user-defined | Custom Item | ✓ | ✓ | ✓ | ✓ | | Custom Item Field | ✓ | ✓ | ✓ | ✓ | | Custom Item Record | ✓ | ✓ | ✓ | ✓ | +| File | ✓ | — | — | — | **Other actions:** `add_project_member`, `clone_project` diff --git a/go.mod b/go.mod index 49223428..e17b5b4b 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/sonh/qs v0.7.0 github.com/teamwork/desksdkgo v1.1.0 github.com/teamwork/spacessdkgo v0.0.0-20260518181558-a6af69d00abb - github.com/teamwork/twapi-go-sdk v1.21.3 + github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e ) require ( diff --git a/go.sum b/go.sum index b4875132..e606bc72 100644 --- a/go.sum +++ b/go.sum @@ -194,6 +194,8 @@ github.com/teamwork/twapi-go-sdk v1.21.2 h1:SsLaxc15Q+m1v+LO0UHWoxCJ+I4wf6uTftfV github.com/teamwork/twapi-go-sdk v1.21.2/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/teamwork/twapi-go-sdk v1.21.3 h1:xTE2l2Xfw9WQLIlWYUNazQM4y4N6K3Y34CJuUO0xTSM= github.com/teamwork/twapi-go-sdk v1.21.3/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= +github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e h1:kaEfM0XTcz0B08P39BFg+e/zg5s2Cd2F2HvTUmVcRKs= +github.com/teamwork/twapi-go-sdk v1.21.4-0.20260815115947-d48d296bd66e/go.mod h1:uj6BNtyyKtggfeY3whzn8bLWuhP1twhghjWD6z3h3F4= github.com/tinylib/msgp v1.6.3 h1:bCSxiTz386UTgyT1i0MSCvdbWjVW+8sG3PjkGsZQt4s= github.com/tinylib/msgp v1.6.3/go.mod h1:RSp0LW9oSxFut3KzESt5Voq4GVWyS+PSulT77roAqEA= github.com/tklauser/go-sysconf v0.3.16 h1:frioLaCQSsF5Cy1jgRBrzr6t502KIIwQ0MArYICU0nA= diff --git a/internal/config/config.go b/internal/config/config.go index 4ed70018..31096521 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -21,6 +21,7 @@ import ( "github.com/getsentry/sentry-go" "github.com/modelcontextprotocol/go-sdk/mcp" desksdk "github.com/teamwork/desksdkgo/client" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/network" "github.com/teamwork/mcp/internal/request" "github.com/teamwork/mcp/internal/toolsets" @@ -234,7 +235,7 @@ func NewMCPServer(resources Resources, groups ...*toolsets.ToolsetGroup) *mcp.Se span.SetTag("mcp.method", method) if callToolParams, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok { span.SetTag("mcp.tool.name", callToolParams.Name) - span.SetTag("mcp.tool.arguments", string(callToolParams.Arguments)) + span.SetTag("mcp.tool.arguments", logsafe.String(string(callToolParams.Arguments))) } if callToolResult, ok := result.(*mcp.CallToolResult); ok { if callToolResult.IsError { @@ -359,7 +360,7 @@ func mcpLoggingMiddleware(resources Resources) mcp.Middleware { if params, ok := req.GetParams().(*mcp.CallToolParamsRaw); ok { attrs = append(attrs, slog.String("mcp.tool.name", params.Name), - slog.String("mcp.tool.arguments", string(params.Arguments)), + slog.String("mcp.tool.arguments", logsafe.String(string(params.Arguments))), ) } diff --git a/internal/logsafe/logsafe.go b/internal/logsafe/logsafe.go new file mode 100644 index 00000000..f5d7f3db --- /dev/null +++ b/internal/logsafe/logsafe.go @@ -0,0 +1,113 @@ +// Package logsafe scrubs request payloads before they reach a log sink or a +// trace tag. +// +// Two things have to happen and the order matters. File content is replaced +// first, so an attachment does not leave part of a customer's document in the +// log; whatever survives is then capped, so an oversized payload of any shape +// cannot fill it. Truncation on its own is not enough, because base64 usually +// leads the arguments object: the retained head would be the readable start of +// the file, and the useful part of the record, the tool name and the other +// parameters, would be what got cut. +package logsafe + +import ( + "bytes" + "fmt" + "regexp" + "strings" +) + +// MaxLoggedBytes caps a scrubbed payload. It matches the cap the response +// writer already applies, so inbound and outbound bodies are treated alike. +const MaxLoggedBytes = 64 << 10 + +const truncatedSuffix = "...[truncated]" + +// contentKeys are the JSON keys whose value is uploaded file content. They are +// upload-specific: "content" is deliberately excluded, since page and comment +// bodies use it for text worth keeping in the log. +var contentKeys = [][]byte{[]byte(`"data"`), []byte(`"fileData"`), []byte(`"file_data"`)} + +// contentValue matches the JSON string value under one of contentKeys. The +// value class is "anything but a quote", so it catches a file of any size, +// including a small one, and is not broken by JSON escaping such as "\/". The +// key anchor means an unrelated field whose value happens to be one of these +// words is untouched. There is no nested quantifier, so the match stays linear +// over a multi-megabyte body. +var contentValue = regexp.MustCompile(`"(data|fileData|file_data)"\s*:\s*"[^"]*"`) + +// Bytes returns b with file content replaced and the result capped. +func Bytes(b []byte) []byte { + if len(b) == 0 { + return b + } + scrubbed := b + if containsContentKey(b) { + scrubbed = contentValue.ReplaceAllFunc(b, func(match []byte) []byte { + key := match[:bytes.IndexByte(match, ':')] + return fmt.Appendf(nil, `%s:""`, key, len(match)) + }) + } + if len(scrubbed) > MaxLoggedBytes { + return append(bytes.Clone(scrubbed[:MaxLoggedBytes]), truncatedSuffix...) + } + return scrubbed +} + +// containsContentKey reports whether b mentions any upload key, so the regex +// scan and its allocation are skipped for the payloads that carry no file, +// which is almost all of them. +func containsContentKey(b []byte) bool { + for _, key := range contentKeys { + if bytes.Contains(b, key) { + return true + } + } + return false +} + +// String is Bytes over a string. +func String(s string) string { + if len(s) == 0 { + return s + } + return string(Bytes([]byte(s))) +} + +// IsTextualContentType reports whether a body of this content type is worth +// capturing in a log. A pending file upload sends raw bytes, and capturing them +// would copy the customer's file into the log and hold a second copy in memory +// for the life of the request. +func IsTextualContentType(contentType string) bool { + if contentType == "" { + // No declared type: the bodies the server sends without one are JSON, and + // anything carrying a file always declares its type. + return true + } + mediaType := contentType + if index := strings.IndexByte(mediaType, ';'); index >= 0 { + mediaType = mediaType[:index] + } + mediaType = strings.ToLower(strings.TrimSpace(mediaType)) + + if strings.HasPrefix(mediaType, "text/") { + return true + } + switch mediaType { + case "application/json", + "application/x-www-form-urlencoded", + "application/xml", + "application/javascript": + return true + } + return strings.HasSuffix(mediaType, "+json") || strings.HasSuffix(mediaType, "+xml") +} + +// ElidedBody is the placeholder logged in place of a body that is not worth +// capturing. +func ElidedBody(length int64, contentType string) string { + if length < 0 { + return fmt.Sprintf("", contentType) + } + return fmt.Sprintf("<%d bytes of %s elided>", length, contentType) +} diff --git a/internal/logsafe/logsafe_test.go b/internal/logsafe/logsafe_test.go new file mode 100644 index 00000000..bf2ad76e --- /dev/null +++ b/internal/logsafe/logsafe_test.go @@ -0,0 +1,119 @@ +package logsafe_test + +import ( + "encoding/base64" + "strings" + "testing" + + "github.com/teamwork/mcp/internal/logsafe" +) + +func TestBytesRedactsFileContent(t *testing.T) { + content := base64.StdEncoding.EncodeToString(make([]byte, 4096)) + payload := `{"name":"twprojects-create_file","arguments":{"data":"` + content + `","name":"plan.md"}}` + + got := logsafe.String(payload) + + if strings.Contains(got, content[:256]) { + t.Error("expected the file content to be redacted") + } + // The point of redacting rather than truncating is that the rest of the + // record survives, so the log still says which tool ran and with what. + if !strings.Contains(got, "twprojects-create_file") { + t.Errorf("expected the tool name to survive redaction, got %q", got) + } + if !strings.Contains(got, `"name":"plan.md"`) { + t.Errorf("expected the other parameters to survive redaction, got %q", got) + } + if !strings.Contains(got, "redacted") { + t.Errorf("expected the placeholder to say what happened, got %q", got) + } +} + +func TestBytesRedactsSmallFile(t *testing.T) { + // A file is redacted whatever its size: a short secret must not survive just + // because its base64 is short. + content := base64.StdEncoding.EncodeToString([]byte("a short secret")) + payload := `{"arguments":{"name":"secret.txt","data":"` + content + `"}}` + + got := logsafe.String(payload) + + if strings.Contains(got, content) { + t.Errorf("expected the file content to be redacted, got %q", got) + } + if !strings.Contains(got, `"name":"secret.txt"`) { + t.Errorf("expected the other parameters to survive, got %q", got) + } +} + +func TestBytesLeavesUnrelatedFieldsAlone(t *testing.T) { + // The key is what's matched, so a field whose value is the word "data", and a + // "content" field (page and comment bodies use it), are both left alone. + payload := `{"arguments":{"search_term":"data","content":"the page body worth keeping"}}` + + if got := logsafe.String(payload); got != payload { + t.Errorf("expected the payload unchanged, got %q", got) + } +} + +func TestBytesTruncatesOversizePayload(t *testing.T) { + // A payload that carries no recognisable content key still must not fill the + // log. + payload := `{"body":"` + strings.Repeat("a", logsafe.MaxLoggedBytes*2) + `"}` + + got := logsafe.String(payload) + + if len(got) > logsafe.MaxLoggedBytes+len("...[truncated]") { + t.Errorf("expected the payload capped, got %d bytes", len(got)) + } + if !strings.HasSuffix(got, "...[truncated]") { + t.Error("expected the payload to say it was truncated") + } +} + +func TestBytesHandlesEmpty(t *testing.T) { + if got := logsafe.Bytes(nil); got != nil { + t.Errorf("expected nil, got %q", got) + } + if got := logsafe.String(""); got != "" { + t.Errorf("expected an empty string, got %q", got) + } +} + +func TestIsTextualContentType(t *testing.T) { + tests := []struct { + contentType string + want bool + }{ + {"", true}, // no declared type: the bodies we send without one are JSON + {"application/json", true}, + {"application/json; charset=utf-8", true}, + {"text/plain", true}, + {"application/vnd.api+json", true}, + {"application/x-www-form-urlencoded", true}, + {"multipart/form-data; boundary=abc", false}, // a file upload + {"application/octet-stream", false}, + {"image/png", false}, + } + + for _, tt := range tests { + t.Run(tt.contentType, func(t *testing.T) { + if got := logsafe.IsTextualContentType(tt.contentType); got != tt.want { + t.Errorf("expected %v for %q, got %v", tt.want, tt.contentType, got) + } + }) + } +} + +// TestBytesStaysLinear guards against a future rewrite of the pattern +// introducing backtracking. A multi-megabyte body runs through this on every +// upload. +func TestBytesStaysLinear(t *testing.T) { + payload := []byte(`{"data":"` + strings.Repeat("A", 8<<20) + `"}`) + + got := logsafe.Bytes(payload) + + if len(got) > logsafe.MaxLoggedBytes+len("...[truncated]") { + t.Errorf("expected the payload capped, got %d bytes", len(got)) + } +} diff --git a/internal/network/roundtripper.go b/internal/network/roundtripper.go index ae70c0ce..699d24b1 100644 --- a/internal/network/roundtripper.go +++ b/internal/network/roundtripper.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/teamwork/mcp/internal/logsafe" "github.com/teamwork/mcp/internal/request" ) @@ -29,14 +30,24 @@ func NewLoggingRoundTripper(logger *slog.Logger, base http.RoundTripper) *Loggin func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, error) { start := time.Now() - var reqBody []byte + // A body that is not text is not worth capturing: a pending file upload + // sends the file itself, so reading it here would copy the customer's file + // into the log and hold a second copy in memory until the request finishes. + // This content-type gate is what keeps file bytes out of the log; the JSON + // bodies that do reach this point carry no inline file content, so they are + // logged as-is. + var loggedRequestBody string if r.Body != nil { - var err error - reqBody, err = io.ReadAll(r.Body) - if err != nil { - lrt.Log.Error("failed to read request body", slog.String("error", err.Error())) + if contentType := r.Header.Get("Content-Type"); !logsafe.IsTextualContentType(contentType) { + loggedRequestBody = logsafe.ElidedBody(r.ContentLength, contentType) + } else { + reqBody, err := io.ReadAll(r.Body) + if err != nil { + lrt.Log.Error("failed to read request body", slog.String("error", err.Error())) + } + r.Body = io.NopCloser(bytes.NewBuffer(reqBody)) + loggedRequestBody = string(reqBody) } - r.Body = io.NopCloser(bytes.NewBuffer(reqBody)) } headers := r.Header.Clone() @@ -59,14 +70,18 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro return resp, err } - var respBody []byte + var loggedResponseBody string if resp.Body != nil { - respBody, err = io.ReadAll(resp.Body) - if err != nil { - lrt.Log.Error("failed to read response body", "error", err) + if contentType := resp.Header.Get("Content-Type"); !logsafe.IsTextualContentType(contentType) { + loggedResponseBody = logsafe.ElidedBody(resp.ContentLength, contentType) + } else { + respBody, err := io.ReadAll(resp.Body) + if err != nil { + lrt.Log.Error("failed to read response body", "error", err) + } + resp.Body = io.NopCloser(bytes.NewBuffer(respBody)) + loggedResponseBody = string(respBody) } - - resp.Body = io.NopCloser(bytes.NewBuffer(respBody)) } info, _ := request.InfoFromContext(r.Context()) @@ -75,10 +90,10 @@ func (lrt *LoggingRoundTripper) RoundTrip(r *http.Request) (*http.Response, erro slog.String("request_url", r.URL.String()), slog.String("request_method", r.Method), slog.Any("request_headers", headers), - slog.String("request_body", string(reqBody)), + slog.String("request_body", loggedRequestBody), slog.Int("response_status", resp.StatusCode), slog.Any("response_headers", resp.Header), - slog.String("response_body", string(respBody)), + slog.String("response_body", loggedResponseBody), slog.String("duration", time.Since(start).String()), slog.Int64("installation.id", info.InstallationID()), slog.String("installation.url", info.InstallationURL()), diff --git a/internal/twprojects/comments.go b/internal/twprojects/comments.go index a8752aad..ce88d180 100644 --- a/internal/twprojects/comments.go +++ b/internal/twprojects/comments.go @@ -128,7 +128,8 @@ func CommentCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "notify": helpers.NotifySchema("Who to notify of the new comment.", true), + "notify": helpers.NotifySchema("Who to notify of the new comment.", true), + "attachment_refs": attachmentRefsSchema("comment"), }, Required: []string{"object", "body"}, }, @@ -149,6 +150,12 @@ func CommentCreate(engine *twapi.Engine) toolsets.ToolWrapper { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil } + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return toolResult, nil + } + commentCreateRequest.PendingFileAttachments = refs + notifyChosen, notifiers, toolResult := parseNotify(arguments, true) if toolResult != nil { return toolResult, nil diff --git a/internal/twprojects/files.go b/internal/twprojects/files.go new file mode 100644 index 00000000..2740a782 --- /dev/null +++ b/internal/twprojects/files.go @@ -0,0 +1,259 @@ +package twprojects + +import ( + "context" + "encoding/base64" + "encoding/json" + "fmt" + "path" + "strings" + + "github.com/google/jsonschema-go/jsonschema" + "github.com/modelcontextprotocol/go-sdk/mcp" + twapi "github.com/teamwork/twapi-go-sdk" + "github.com/teamwork/twapi-go-sdk/projects" + + "github.com/teamwork/mcp/internal/helpers" + "github.com/teamwork/mcp/internal/toolsets" +) + +// List of methods available in the Teamwork.com MCP service. +// +// The naming convention for methods follows a pattern described here: +// https://github.com/github/github-mcp-server/issues/333 +const ( + MethodFileCreate toolsets.Method = "twprojects-create_file" +) + +// maxAttachmentBytes caps the decoded size of an inline attachment. +// +// The hard ceiling is the HTTP server's maximum request body, which covers the +// whole JSON-RPC envelope: base64 grows by four thirds, so 5 MB of file already +// costs about 6.7 MB on the wire before JSON escaping and the rest of the +// message. Rejecting here rather than at the transport turns an unreadable +// connection reset into a tool result the caller can act on. +// +// In practice the binding limit is far lower. The caller has to emit the base64 +// itself, at roughly one output token per three bytes of file, so a megabyte is +// already out of reach. This limit exists to keep the server standing, not to +// describe what is usable. +const maxAttachmentBytes = 5 << 20 + +// maxFileNameBytes bounds the stored name. Most filesystems stop around 255 +// bytes; the extension is preserved when truncating so the file still opens +// with the right application. +const maxFileNameBytes = 200 + +// attachmentRefsSchema returns the schema for the pending file references +// parameter. The entity is named in the description so the caller knows what it +// is attaching to. +// +// There is no minimum length: a caller that sends an empty array to mean "none" +// gets a no-op rather than a validation failure. +func attachmentRefsSchema(entity string) *jsonschema.Schema { + return &jsonschema.Schema{ + Description: fmt.Sprintf( + "References of files to attach to the %s, as returned by %s. Each looks like "+ + "\"tf_1a2b\" and can only be used once, so upload a file for each place it "+ + "should go. Files are added to whatever is already attached; nothing is removed.", + entity, MethodFileCreate), + AnyOf: []*jsonschema.Schema{ + {Type: "array", Items: &jsonschema.Schema{Type: "string"}}, + {Type: "null"}, + }, + } +} + +// fileCreateResult is what the tool hands back. Reference is the value the +// attachment parameters take. +type fileCreateResult struct { + Reference string `json:"reference"` + Name string `json:"name"` + Size int64 `json:"size"` + Usage string `json:"usage"` +} + +// FileCreate uploads a file to Teamwork.com so that it can be attached to +// something. +func FileCreate(engine *twapi.Engine) toolsets.ToolWrapper { + return toolsets.ToolWrapper{ + Tool: &mcp.Tool{ + Name: string(MethodFileCreate), + Description: fmt.Sprintf("Upload a file so it can be attached to a task, comment or message. "+ + "Returns a single-use reference like \"tf_1a2b\"; pass it in attachment_refs on %s, %s, "+ + "%s or %s. Content is sent inline as base64, so this suits text you generated, such as "+ + "plans, specs or CSV, rather than large binaries.", + MethodTaskCreate, MethodTaskUpdate, MethodCommentCreate, MethodMessageCreate), + Annotations: &mcp.ToolAnnotations{ + Title: "Create File", + // The upload goes to storage Teamwork.com manages and the file lands + // in the caller's own account, so nothing here reaches outside it. + DestructiveHint: new(false), + OpenWorldHint: new(false), + }, + InputSchema: &jsonschema.Schema{ + Type: "object", + Properties: map[string]*jsonschema.Schema{ + "name": { + Type: "string", + MinLength: new(1), + Description: "The file name, including its extension, for example \"plan.md\". " + + "Teamwork.com works out how to display the file from the extension, so a name " + + "without one is harder to open. Any directory part is removed.", + }, + "data": { + Type: "string", + MaxLength: new(base64.StdEncoding.EncodedLen(maxAttachmentBytes)), + Description: fmt.Sprintf("The file content, base64-encoded with the standard "+ + "alphabet. It must decode to between 1 byte and %d bytes.", maxAttachmentBytes), + }, + }, + Required: []string{"name", "data"}, + }, + }, + Handler: func(ctx context.Context, request *mcp.CallToolRequest) (*mcp.CallToolResult, error) { + var arguments map[string]any + if err := json.Unmarshal(request.Params.Arguments, &arguments); err != nil { + return helpers.NewToolResultTextError("failed to decode request: %s", err.Error()), nil + } + + var name, data string + if err := helpers.ParamGroup(arguments, + helpers.RequiredParam(&name, "name"), + helpers.RequiredParam(&data, "data"), + ); err != nil { + return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil + } + + name, err := sanitizeFileName(name) + if err != nil { + return helpers.NewToolResultTextError("invalid name: %s", err.Error()), nil + } + + // Check the encoded length first: decoding allocates the payload a + // second time, and there is no reason to pay for that only to reject + // the result. + if len(data) > base64.StdEncoding.EncodedLen(maxAttachmentBytes) { + return helpers.NewToolResultTextError( + "file is too large: %d base64 characters exceed the %d byte limit on decoded "+ + "content. Upload a smaller file, or split the content across several.", + len(data), maxAttachmentBytes), nil + } + + // Not an API failure: the caller supplied malformed base64, so report + // it as a tool result it can correct rather than as a transport error. + content, err := base64.StdEncoding.DecodeString(data) + if err != nil { + return helpers.NewToolResultTextError("failed to decode base64 data: %s", err.Error()), nil + } + switch { + case len(content) == 0: + return helpers.NewToolResultTextError( + "the data decoded to zero bytes, so there is nothing to upload"), nil + case len(content) > maxAttachmentBytes: + return helpers.NewToolResultTextError( + "file is too large: %d bytes exceed the %d byte limit. Upload a smaller file, "+ + "or split the content across several.", + len(content), maxAttachmentBytes), nil + } + + pendingFile, err := projects.PendingFileCreate(ctx, engine, + projects.NewPendingFileCreateRequest(name, content)) + if err != nil { + return helpers.HandleAPIError(err, "failed to upload file") + } + + return helpers.NewToolResultJSON(fileCreateResult{ + Reference: string(pendingFile.PendingFile.Ref), + Name: name, + Size: int64(len(content)), + Usage: fmt.Sprintf("Pass %q in attachment_refs on %s, %s, %s or %s.", + pendingFile.PendingFile.Ref, MethodTaskCreate, MethodTaskUpdate, + MethodCommentCreate, MethodMessageCreate), + }) + }, + } +} + +// parseAttachmentRefs reads the attachment references shared by the tools that +// can attach a file. It returns nil when the caller named none, so that the +// request omits the field entirely rather than sending an empty set. +func parseAttachmentRefs(arguments map[string]any) ([]projects.PendingFileRef, *mcp.CallToolResult) { + var refs []projects.PendingFileRef + if err := helpers.ParamGroup(arguments, + helpers.OptionalListParam(&refs, "attachment_refs"), + ); err != nil { + return nil, helpers.NewToolResultTextError("invalid attachment_refs: %s", err.Error()) + } + + cleaned := make([]projects.PendingFileRef, 0, len(refs)) + for _, ref := range refs { + if trimmed := projects.PendingFileRef(strings.TrimSpace(string(ref))); trimmed != "" { + cleaned = append(cleaned, trimmed) + } + } + if len(cleaned) == 0 { + return nil, nil + } + return cleaned, nil +} + +// parseTaskAttachments reads the attachment references for the task tools, +// which take the structured form rather than a plain list. +func parseTaskAttachments(arguments map[string]any) (*projects.TaskAttachments, *mcp.CallToolResult) { + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return nil, toolResult + } + if len(refs) == 0 { + return nil, nil + } + + attachments := projects.TaskAttachments{ + PendingFiles: make([]projects.TaskAttachmentPendingFile, 0, len(refs)), + } + for _, ref := range refs { + attachments.PendingFiles = append(attachments.PendingFiles, + projects.TaskAttachmentPendingFile{Reference: ref}) + } + return &attachments, nil +} + +// sanitizeFileName reduces a caller-supplied name to something safe to store as +// an attachment name. +// +// Models emit paths rather than names, and both "docs/plan.md" and a Windows +// style path turn up. path.Base only understands the forward slash, so the +// backslash form has to be removed explicitly. +func sanitizeFileName(name string) (string, error) { + name = strings.TrimSpace(name) + if index := strings.LastIndexAny(name, `/\`); index >= 0 { + name = name[index+1:] + } + // Control characters would break any downstream header carrying the name. + name = strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f { + return -1 + } + return r + }, name) + name = strings.TrimSpace(name) + + switch name { + case "", ".", "..": + return "", fmt.Errorf("name must be a file name, not a path") + } + + if len(name) > maxFileNameBytes { + // Keep the extension so the file still opens with the right application, + // and cut the stem on a rune boundary so the name stays valid UTF-8. + extension := path.Ext(name) + if len(extension) > maxFileNameBytes/2 { + extension = "" // a late dot, not an extension + } + stem := name[:len(name)-len(extension)] + stem = strings.ToValidUTF8(stem[:maxFileNameBytes-len(extension)], "") + name = stem + extension + } + return name, nil +} diff --git a/internal/twprojects/files_test.go b/internal/twprojects/files_test.go new file mode 100644 index 00000000..0439f49e --- /dev/null +++ b/internal/twprojects/files_test.go @@ -0,0 +1,291 @@ +package twprojects_test + +import ( + "encoding/base64" + "encoding/json" + "net/http" + "strings" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/teamwork/mcp/internal/testutil" + "github.com/teamwork/mcp/internal/twprojects" +) + +func TestFileCreate(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": "plan.md", + "data": base64.StdEncoding.EncodeToString([]byte("# Plan\n")), + }) + + // The upload is multipart rather than JSON, so the assertion is on the part + // itself: the file has to arrive under the name the API expects, carrying the + // decoded bytes rather than the base64 the caller sent. + body := string(*requestBody) + if !strings.Contains(body, `name="file"`) { + t.Errorf("expected a form part named file, got %q", body) + } + if !strings.Contains(body, `filename="plan.md"`) { + t.Errorf("expected the file name in the body, got %q", body) + } + if !strings.Contains(body, "# Plan") { + t.Errorf("expected the decoded contents in the body, got %q", body) + } + if strings.Contains(body, base64.StdEncoding.EncodeToString([]byte("# Plan\n"))) { + t.Errorf("expected the contents to be decoded before upload, got %q", body) + } +} + +func TestFileCreateSanitizesFileName(t *testing.T) { + tests := []struct { + name string + input string + want string + }{{ + name: "posix path", + input: "docs/plans/plan.md", + want: "plan.md", + }, { + name: "windows path", + input: `C:\Users\someone\plan.md`, + want: "plan.md", + }, { + name: "traversal", + input: "../../etc/passwd", + want: "passwd", + }, { + name: "control characters", + input: "pl\nan\r.md", + want: "plan.md", + }, { + name: "surrounding whitespace", + input: " plan.md ", + want: "plan.md", + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": tt.input, + "data": base64.StdEncoding.EncodeToString([]byte("contents")), + }) + + // The leading filename=" anchors the check to the multipart part's + // name, so an un-sanitized path could not match by appearing elsewhere. + want := `filename="` + tt.want + `"` + if !strings.Contains(string(*requestBody), want) { + t.Errorf("expected the body to carry %s, got %q", want, string(*requestBody)) + } + }) + } +} + +func TestFileCreateRejectsBadInput(t *testing.T) { + tests := []struct { + name string + arguments map[string]any + wantIn string + }{{ + name: "malformed base64", + arguments: map[string]any{"name": "plan.md", "data": "not base64!!!"}, + wantIn: "base64", + }, { + name: "empty content", + arguments: map[string]any{"name": "plan.md", "data": ""}, + wantIn: "zero bytes", + }, { + name: "name is only a path", + arguments: map[string]any{"name": "../", "data": base64.StdEncoding.EncodeToString([]byte("x"))}, + wantIn: "file name", + }} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // A caller mistake has to come back as a tool result it can correct, + // not as a transport error. + mcpServer := mcpServerMock(t, http.StatusCreated, []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), tt.arguments, + testutil.ExecuteToolRequestWithCheckMessage(func(t *testing.T, result mcp.Result) { + t.Helper() + assertErrorResultContains(t, result, tt.wantIn) + }), + ) + }) + } +} + +func TestFileCreateRejectsOversizeBeforeUploading(t *testing.T) { + // The size check has to run before the upload, so an oversize payload never + // reaches the API. + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"pendingFile":{"ref":"tf_1a2b"}}`)) + + oversize := base64.StdEncoding.EncodeToString(make([]byte, (5<<20)+1)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodFileCreate.String(), map[string]any{ + "name": "big.bin", + "data": oversize, + }, + testutil.ExecuteToolRequestWithCheckMessage(func(t *testing.T, result mcp.Result) { + t.Helper() + assertErrorResultContains(t, result, "too large") + }), + ) + + if len(*requestBody) != 0 { + t.Errorf("expected no request to be sent, got a body of %d bytes", len(*requestBody)) + } +} + +// assertErrorResultContains checks that a tool call came back as an error result +// the caller can read, rather than as a transport error. +func assertErrorResultContains(t *testing.T, result mcp.Result, want string) { + t.Helper() + + toolResult, ok := result.(*mcp.CallToolResult) + if !ok { + t.Fatalf("unexpected result type: %T", result) + } + if !toolResult.IsError { + t.Fatalf("expected an error tool result, got %+v", toolResult) + } + if len(toolResult.Content) == 0 { + t.Fatal("error tool result should carry content the model can read") + } + textContent, ok := toolResult.Content[0].(*mcp.TextContent) + if !ok { + t.Fatalf("unexpected content type: %T", toolResult.Content[0]) + } + if !strings.Contains(strings.ToLower(textContent.Text), want) { + t.Errorf("expected the message to mention %q, got %q", want, textContent.Text) + } +} + +func TestTaskCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"task":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTaskCreate.String(), map[string]any{ + "name": "example", + "tasklist_id": float64(777), + "attachment_refs": []any{"tf_A", "tf_B"}, + }) + + // attachments is a sibling of task in the request body, not one of its + // attributes, so the nesting is what this pins. + var payload struct { + Attachments struct { + PendingFiles []struct { + Reference string `json:"reference"` + } `json:"pendingFiles"` + } `json:"attachments"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Attachments.PendingFiles) != 2 { + t.Fatalf("expected two pending files, got body %q", string(*requestBody)) + } + if payload.Attachments.PendingFiles[0].Reference != "tf_A" || + payload.Attachments.PendingFiles[1].Reference != "tf_B" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} + +func TestTaskUpdateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusOK, []byte(`{}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodTaskUpdate.String(), map[string]any{ + "id": float64(123), + "attachment_refs": []any{"tf_A"}, + }) + + if !strings.Contains(string(*requestBody), `"reference":"tf_A"`) { + t.Errorf("expected the reference in the body, got %q", string(*requestBody)) + } +} + +func TestTaskAttachmentsOmittedWhenNotRequested(t *testing.T) { + // An empty attachments object would be a payload change for every caller + // that attaches nothing, so the key has to stay absent. + for _, tt := range []struct { + name string + method string + status int + arguments map[string]any + }{{ + name: "create", + method: twprojects.MethodTaskCreate.String(), + status: http.StatusCreated, + arguments: map[string]any{"name": "example", "tasklist_id": float64(777)}, + }, { + name: "update", + method: twprojects.MethodTaskUpdate.String(), + status: http.StatusOK, + arguments: map[string]any{"id": float64(123), "name": "example"}, + }} { + t.Run(tt.name, func(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, tt.status, + []byte(`{"task":{"id":123}}`)) + testutil.ExecuteToolRequest(t, mcpServer, tt.method, tt.arguments) + + var payload map[string]any + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if _, ok := payload["attachments"]; ok { + t.Errorf("expected no attachments key, got body %q", string(*requestBody)) + } + if _, ok := payload["attachmentOptions"]; ok { + t.Errorf("expected no attachmentOptions key, got body %q", string(*requestBody)) + } + }) + } +} + +func TestCommentCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, []byte(`{"id":"123"}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodCommentCreate.String(), map[string]any{ + "object": map[string]any{"type": "tasks", "id": float64(777)}, + "body": "see attached", + "attachment_refs": []any{"tf_A", "tf_B"}, + }) + + var payload struct { + Comment struct { + PendingFileAttachments []string `json:"pendingFileAttachments"` + } `json:"comment"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Comment.PendingFileAttachments) != 2 || + payload.Comment.PendingFileAttachments[0] != "tf_A" || + payload.Comment.PendingFileAttachments[1] != "tf_B" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} + +func TestMessageCreateSendsAttachments(t *testing.T) { + mcpServer, requestBody := mcpServerMockWithRequestBody(t, http.StatusCreated, + []byte(`{"messageId":"123"}`)) + testutil.ExecuteToolRequest(t, mcpServer, twprojects.MethodMessageCreate.String(), map[string]any{ + "project_id": float64(777), + "title": "example", + "body": "see attached", + "attachment_refs": []any{"tf_A"}, + }) + + var payload struct { + Post struct { + PendingFileAttachments []string `json:"pendingFileAttachments"` + } `json:"post"` + } + if err := json.Unmarshal(*requestBody, &payload); err != nil { + t.Fatalf("failed to decode request body %q: %v", string(*requestBody), err) + } + if len(payload.Post.PendingFileAttachments) != 1 || payload.Post.PendingFileAttachments[0] != "tf_A" { + t.Errorf("unexpected references in body %q", string(*requestBody)) + } +} diff --git a/internal/twprojects/messages.go b/internal/twprojects/messages.go index 13f25f93..426fbc15 100644 --- a/internal/twprojects/messages.go +++ b/internal/twprojects/messages.go @@ -81,7 +81,8 @@ func MessageCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "notify": helpers.NotifySchema("Who to notify of the new message.", false), + "notify": helpers.NotifySchema("Who to notify of the new message.", false), + "attachment_refs": attachmentRefsSchema("message"), }, Required: []string{"title", "project_id", "body"}, }, @@ -103,6 +104,12 @@ func MessageCreate(engine *twapi.Engine) toolsets.ToolWrapper { return helpers.NewToolResultTextError("invalid parameters: %s", err.Error()), nil } + refs, toolResult := parseAttachmentRefs(arguments) + if toolResult != nil { + return toolResult, nil + } + messageCreateRequest.PendingFileAttachments = refs + notifyChosen, notifiers, toolResult := parseNotify(arguments, false) if toolResult != nil { return toolResult, nil diff --git a/internal/twprojects/tasks.go b/internal/twprojects/tasks.go index 5f838655..e3bd7162 100644 --- a/internal/twprojects/tasks.go +++ b/internal/twprojects/tasks.go @@ -114,8 +114,9 @@ func TaskCreate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "assignees": helpers.UserGroupsSchema("Assignees for the task.", false), - "tag_ids": helpers.TagIDsAssociateSchema("task"), + "assignees": helpers.UserGroupsSchema("Assignees for the task.", false), + "tag_ids": helpers.TagIDsAssociateSchema("task"), + "attachment_refs": attachmentRefsSchema("task"), "predecessors": { Description: "Task dependencies that must be completed before this task can start.", AnyOf: []*jsonschema.Schema{ @@ -184,6 +185,15 @@ func TaskCreate(engine *twapi.Engine) toolsets.ToolWrapper { taskCreateRequest.Assignees = assignees } + // Only set attachments when the caller named one: the field is a + // sibling of the task in the request body, so an empty one would be a + // payload change for every caller that attaches nothing. + if attachments, toolResult := parseTaskAttachments(arguments); toolResult != nil { + return toolResult, nil + } else if attachments != nil { + taskCreateRequest.Attachments = *attachments + } + if predecessors, ok := arguments["predecessors"]; ok { predecessorsSlice, ok := predecessors.([]any) if !ok { @@ -345,7 +355,8 @@ func TaskUpdate(engine *twapi.Engine) toolsets.ToolWrapper { {Type: "null"}, }, }, - "tag_ids": helpers.TagIDsAssociateSchema("task"), + "tag_ids": helpers.TagIDsAssociateSchema("task"), + "attachment_refs": attachmentRefsSchema("task"), "predecessors": { Description: "Task dependencies that must be completed before this task can start.", AnyOf: []*jsonschema.Schema{ @@ -452,6 +463,14 @@ func TaskUpdate(engine *twapi.Engine) toolsets.ToolWrapper { taskUpdateRequest.Assignees = assignees } + // Only set attachments when the caller named one. Attaching is additive + // server side, so this never disturbs the files the task already has. + if attachments, toolResult := parseTaskAttachments(arguments); toolResult != nil { + return toolResult, nil + } else if attachments != nil { + taskUpdateRequest.Attachments = *attachments + } + if clearAssignees { // Empty arrays unassign every assignee dimension. Job roles are // included because the SDK now sends the "Jobroles-Enabled: true" diff --git a/internal/twprojects/tools.go b/internal/twprojects/tools.go index f3b4e20e..cdbdc0ae 100644 --- a/internal/twprojects/tools.go +++ b/internal/twprojects/tools.go @@ -45,6 +45,7 @@ func DefaultToolsetGroup(readOnly, allowDelete bool, engine *twapi.Engine) *tool // --- projects sub-toolset --- projectsWriteTools := []toolsets.ToolWrapper{ + FileCreate(engine), ProjectCategoryCreate(engine), ProjectCategoryUpdate(engine), ProjectClone(engine),