diff --git a/internal/clientid/clientid.go b/internal/clientid/clientid.go new file mode 100644 index 0000000..290fdb3 --- /dev/null +++ b/internal/clientid/clientid.go @@ -0,0 +1,92 @@ +// Package clientid resolves the client identity the MCP sends to the agent. +// +// The formae CLI persists a per-machine ID at ~/.pel/formae/cli_client_id, +// created by the CLI itself. The MCP sends the same ID so commands issued +// through it are attributed to the same client as the user's own CLI. This +// package never creates the file: when it is missing or unreadable, it +// degrades to the Fallback constant. +package clientid + +import ( + "os" + "path/filepath" + "strings" + "sync" +) + +// Fallback is sent when no CLI client ID can be resolved. It matches the +// constant the MCP historically sent, so agents see no new identity when +// resolution fails. +const Fallback = "formae-mcp" + +// maxIDLen bounds accepted IDs. A KSUID is 27 bytes; the bound is generous so +// a future formae ID format still passes without coupling to the exact shape. +const maxIDLen = 64 + +// Resolver resolves the CLI client ID. Filesystem access is injected so the +// logic is unit-testable. Safe for concurrent use. +type Resolver struct { + Home func() (string, error) + ReadFile func(string) ([]byte, error) + + mu sync.Mutex + cached string +} + +// NewResolver wires a Resolver to the real filesystem. +func NewResolver() *Resolver { + return &Resolver{ + Home: os.UserHomeDir, + ReadFile: os.ReadFile, + } +} + +// Resolve returns the CLI client ID, or Fallback when it cannot be read. It +// never fails a caller: an unresolvable ID degrades to Fallback. A +// successfully read ID is cached for the process lifetime (the file never +// changes once written); a fallback is not cached, so a later call picks up +// the real file once it exists. +func (r *Resolver) Resolve() string { + r.mu.Lock() + defer r.mu.Unlock() + if r.cached != "" { + return r.cached + } + if id, ok := r.read(); ok { + r.cached = id + return id + } + return Fallback +} + +func (r *Resolver) read() (string, bool) { + home, err := r.Home() + if err != nil { + return "", false + } + data, err := r.ReadFile(filepath.Join(home, ".pel", "formae", "cli_client_id")) + if err != nil { + return "", false + } + id := strings.TrimSpace(string(data)) + if !validID(id) { + return "", false + } + return id, true +} + +// validID reports whether id is safe to send as an HTTP header value: 1-64 +// bytes of printable ASCII with no whitespace or control characters. Go's +// HTTP transport rejects requests whose header values contain control +// characters, so an unvalidated corrupt file would fail every command. +func validID(id string) bool { + if len(id) == 0 || len(id) > maxIDLen { + return false + } + for i := 0; i < len(id); i++ { + if id[i] <= 0x20 || id[i] >= 0x7f { + return false + } + } + return true +} diff --git a/internal/clientid/clientid_test.go b/internal/clientid/clientid_test.go new file mode 100644 index 0000000..f916bdb --- /dev/null +++ b/internal/clientid/clientid_test.go @@ -0,0 +1,149 @@ +package clientid + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// newTestResolver returns a Resolver rooted at dir. +func newTestResolver(dir string) *Resolver { + return &Resolver{ + Home: func() (string, error) { return dir, nil }, + ReadFile: os.ReadFile, + } +} + +// writeIDFile creates /.pel/formae/cli_client_id with the given content. +func writeIDFile(t *testing.T, dir, content string) { + t.Helper() + idDir := filepath.Join(dir, ".pel", "formae") + if err := os.MkdirAll(idDir, 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(idDir, "cli_client_id"), []byte(content), 0o600); err != nil { + t.Fatal(err) + } +} + +func TestResolveReturnsTrimmedFileContent(t *testing.T) { + dir := t.TempDir() + writeIDFile(t, dir, "2N3x8aQdLmVp0rGhTzYwBcKfJe1\n") + r := newTestResolver(dir) + if got := r.Resolve(); got != "2N3x8aQdLmVp0rGhTzYwBcKfJe1" { + t.Fatalf("got %q", got) + } +} + +func TestResolveAcceptsExactly64Bytes(t *testing.T) { + dir := t.TempDir() + id := strings.Repeat("a", 64) + writeIDFile(t, dir, id) + r := newTestResolver(dir) + if got := r.Resolve(); got != id { + t.Fatalf("got %q, want the 64-byte id accepted", got) + } +} + +func TestResolveRejectsInvalidContent(t *testing.T) { + cases := map[string]string{ + "empty": "", + "whitespace only": " \n\t", + "embedded newline": "abc\ndef", + "embedded space": "abc def", + "control char": "abc\x01def", + "DEL byte": "abc\x7fdef", + "non-ascii": "abc\xc3\xa9def", + // printable so only the length bound fails + "over 64 bytes": strings.Repeat("a", 65), + } + + for name, content := range cases { + t.Run(name, func(t *testing.T) { + dir := t.TempDir() + writeIDFile(t, dir, content) + r := newTestResolver(dir) + if got := r.Resolve(); got != Fallback { + t.Fatalf("got %q, want fallback", got) + } + }) + } +} + +func TestResolveMissingFileFallsBack(t *testing.T) { + dir := t.TempDir() + r := newTestResolver(dir) + if got := r.Resolve(); got != Fallback { + t.Fatalf("got %q, want fallback", got) + } +} + +func TestResolveFallsBackWhenHomeUnavailable(t *testing.T) { + r := &Resolver{ + Home: func() (string, error) { return "", os.ErrNotExist }, + ReadFile: os.ReadFile, + } + if got := r.Resolve(); got != Fallback { + t.Fatalf("got %q, want fallback", got) + } +} + +func TestResolveCachesSuccessfulRead(t *testing.T) { + dir := t.TempDir() + writeIDFile(t, dir, "2N3x8aQdLmVp0rGhTzYwBcKfJe1") + reads := 0 + r := newTestResolver(dir) + realRead := r.ReadFile + r.ReadFile = func(p string) ([]byte, error) { + reads++ + return realRead(p) + } + first := r.Resolve() + second := r.Resolve() + if first != second || first != "2N3x8aQdLmVp0rGhTzYwBcKfJe1" { + t.Fatalf("got %q then %q", first, second) + } + if reads != 1 { + t.Fatalf("file read %d times, want 1", reads) + } +} + +func TestResolveDoesNotCacheFallback(t *testing.T) { + dir := t.TempDir() + r := newTestResolver(dir) + if got := r.Resolve(); got != Fallback { + t.Fatalf("got %q, want fallback", got) + } + // the file appearing later must win over a previous fallback + writeIDFile(t, dir, "2N3x8aQdLmVp0rGhTzYwBcKfJe1") + if got := r.Resolve(); got != "2N3x8aQdLmVp0rGhTzYwBcKfJe1" { + t.Fatalf("got %q, want file content", got) + } +} + +func TestResolveConcurrentUse(t *testing.T) { + dir := t.TempDir() + writeIDFile(t, dir, "2N3x8aQdLmVp0rGhTzYwBcKfJe1") + r := newTestResolver(dir) + done := make(chan string, 8) + for i := 0; i < 8; i++ { + go func() { done <- r.Resolve() }() + } + for i := 0; i < 8; i++ { + if got := <-done; got != "2N3x8aQdLmVp0rGhTzYwBcKfJe1" { + t.Fatalf("got %q", got) + } + } +} + +func TestNewResolverUsesRealHomeAndReadFile(t *testing.T) { + r := NewResolver() + if r.Home == nil || r.ReadFile == nil { + t.Fatal("NewResolver must wire Home and ReadFile") + } + // NewResolver must never crash even against a real, likely-fileless home. + if got := r.Resolve(); got == "" { + t.Fatal("Resolve must never return an empty string") + } +} diff --git a/internal/server/resources.go b/internal/server/resources.go index 205f6d8..997ab9e 100644 --- a/internal/server/resources.go +++ b/internal/server/resources.go @@ -192,7 +192,8 @@ Queries use field:value pairs separated by spaces. Multiple pairs are AND-combin | Field | Type | Description | Example | |-------|------|-------------|---------| | id | string | Command ID | id:abc123 | -| client | string | Client ID | client:me | +| client | string | Client ID (this machine's formae client id, shared with the CLI when its id file exists, else 'formae-mcp'; not the human) | client:me | +| user | string | User (human); 'me' resolves to the bearer token's subject, a UUID matches the subject id, anything else matches the display name | user:me | | command | string | Command type | command:apply | | status | string | Command state | status:in_progress | | stack | string | Stack name | stack:production | @@ -204,6 +205,8 @@ Queries use field:value pairs separated by spaces. Multiple pairs are AND-combin - S3 buckets in production: type:AWS::S3::Bucket stack:production - Failed commands: status:failed - Running commands: status:in_progress +- Commands sent from this machine (CLI or MCP, same client id): client:me +- Commands from the human on the other end of this session: user:me ` const conceptsDoc = `# Formae Core Concepts diff --git a/internal/server/server.go b/internal/server/server.go index 70b096e..f6372f8 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -10,6 +10,7 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/platform-engineering-labs/formae-mcp/internal/clientid" "github.com/platform-engineering-labs/formae-mcp/internal/config" "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" "github.com/platform-engineering-labs/formae-mcp/internal/profile" @@ -33,7 +34,8 @@ func implementation() *mcp.Implementation { type Server struct { mcpServer *mcp.Server hub *HubClient - forcedEndpoint string // when set, empty-profile calls use this (tests / explicit) + forcedEndpoint string // when set, empty-profile calls use this (tests / explicit) + clientID *clientid.Resolver // resolves the Client-ID header value } // New creates a new formae MCP server connected to the given agent endpoint. @@ -49,6 +51,7 @@ func New(endpoint string) *Server { mcpServer: mcpServer, hub: NewHubClient(), forcedEndpoint: endpoint, + clientID: clientid.NewResolver(), } s.registerTools() @@ -312,7 +315,7 @@ func (s *Server) handleGetCommandStatus(_ context.Context, _ *mcp.CallToolReques if err != nil { return errorResult(err), nil, nil } - result, err := c.GetCommandStatus(input.CommandID, "formae-mcp") + result, err := c.GetCommandStatus(input.CommandID, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } @@ -328,7 +331,7 @@ func (s *Server) handleListCommands(_ context.Context, _ *mcp.CallToolRequest, i if err != nil { return errorResult(err), nil, nil } - result, err := c.ListCommands(input.Query, maxResults, "formae-mcp") + result, err := c.ListCommands(input.Query, maxResults, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } @@ -558,7 +561,7 @@ func (s *Server) handleApplyForma(_ context.Context, _ *mcp.CallToolRequest, inp if err != nil { return errorResult(err), nil, nil } - result, err := c.SubmitCommand("apply", input.Mode, input.Simulate, input.Force, formaJSON, "formae-mcp") + result, err := c.SubmitCommand("apply", input.Mode, input.Simulate, input.Force, formaJSON, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } @@ -587,7 +590,7 @@ func (s *Server) handleDestroyForma(_ context.Context, _ *mcp.CallToolRequest, i } if input.Query != "" { - result, err := c.DestroyByQuery(input.Query, input.Simulate, "formae-mcp") + result, err := c.DestroyByQuery(input.Query, input.Simulate, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } @@ -599,7 +602,7 @@ func (s *Server) handleDestroyForma(_ context.Context, _ *mcp.CallToolRequest, i return errorResult(fmt.Errorf("failed to evaluate forma file: %w", err)), nil, nil } - result, err := c.SubmitCommand("destroy", "", input.Simulate, false, formaJSON, "formae-mcp") + result, err := c.SubmitCommand("destroy", "", input.Simulate, false, formaJSON, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } @@ -611,7 +614,7 @@ func (s *Server) handleCancelCommands(_ context.Context, _ *mcp.CallToolRequest, if err != nil { return errorResult(err), nil, nil } - result, err := c.CancelCommands(input.Query, "formae-mcp") + result, err := c.CancelCommands(input.Query, s.clientID.Resolve()) if err != nil { return errorResult(err), nil, nil } diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 1868c85..89c9071 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -6,14 +6,41 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "strings" "testing" "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/platform-engineering-labs/formae-mcp/internal/clientid" "github.com/platform-engineering-labs/formae-mcp/internal/featuregate" ) +// withClientIDFile points HOME at a temp dir holding a formae CLI client ID +// file, so a test can assert that ID is the one threaded into the Client-ID +// header. Returns the ID. +func withClientIDFile(t *testing.T) string { + t.Helper() + dir := t.TempDir() + idDir := filepath.Join(dir, ".pel", "formae") + if err := os.MkdirAll(idDir, 0o700); err != nil { + t.Fatal(err) + } + const id = "2N3x8aQdLmVp0rGhTzYwBcKfJe1" + if err := os.WriteFile(filepath.Join(idDir, "cli_client_id"), []byte(id+"\n"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("HOME", dir) + return id +} + +// withoutClientIDFile points HOME at an empty temp dir, so a test can assert +// the Client-ID header falls back to clientid.Fallback. +func withoutClientIDFile(t *testing.T) { + t.Helper() + t.Setenv("HOME", t.TempDir()) +} + // mockAgent creates a test HTTP server that simulates the formae agent. // The handler map keys are "METHOD /path" strings. func mockAgent(t *testing.T, handlers map[string]http.HandlerFunc) *httptest.Server { @@ -265,8 +292,11 @@ func TestGetAgentStats(t *testing.T) { } func TestGetCommandStatus(t *testing.T) { + wantID := withClientIDFile(t) + var gotClientID string agent := mockAgent(t, map[string]http.HandlerFunc{ "GET /api/v1/commands/status": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") id := r.URL.Query().Get("id") if id == "cmd-123" { _, _ = fmt.Fprint(w, `{"id":"cmd-123","status":"completed"}`) @@ -290,6 +320,9 @@ func TestGetCommandStatus(t *testing.T) { if result.IsError { t.Fatalf("expected success, got error: %s", textContent(t, result)) } + if gotClientID != wantID { + t.Fatalf("Client-ID header = %q, want %q", gotClientID, wantID) + } }) t.Run("missing command_id rejected by schema", func(t *testing.T) { @@ -303,8 +336,11 @@ func TestGetCommandStatus(t *testing.T) { } func TestListCommands(t *testing.T) { + wantID := withClientIDFile(t) + var gotClientID string agent := mockAgent(t, map[string]http.HandlerFunc{ "GET /api/v1/commands/status": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") _, _ = fmt.Fprint(w, `{"Commands":[{"id":"cmd-1","status":"completed"}]}`) }, }) @@ -320,13 +356,49 @@ func TestListCommands(t *testing.T) { if result.IsError { t.Fatalf("expected success, got error: %s", textContent(t, result)) } + if gotClientID != wantID { + t.Fatalf("Client-ID header = %q, want %q", gotClientID, wantID) + } +} + +// TestListCommands_ClientIDFallsBackWithoutCLIFile covers all six call sites +// via one representative endpoint: when the CLI hasn't written its client ID +// file, the MCP still sends the historical "formae-mcp" literal rather than +// failing the command. +func TestListCommands_ClientIDFallsBackWithoutCLIFile(t *testing.T) { + withoutClientIDFile(t) + var gotClientID string + agent := mockAgent(t, map[string]http.HandlerFunc{ + "GET /api/v1/commands/status": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") + _, _ = fmt.Fprint(w, `{"Commands":[]}`) + }, + }) + defer agent.Close() + + session := connectTestServer(t, agent.URL) + result, err := session.CallTool(context.Background(), &mcp.CallToolParams{ + Name: "list_commands", + }) + if err != nil { + t.Fatalf("CallTool failed: %v", err) + } + if result.IsError { + t.Fatalf("expected success, got error: %s", textContent(t, result)) + } + if gotClientID != clientid.Fallback { + t.Fatalf("Client-ID header = %q, want fallback %q", gotClientID, clientid.Fallback) + } } // --- Mutation tool tests --- func TestCancelCommands(t *testing.T) { + wantID := withClientIDFile(t) + var gotClientID string agent := mockAgent(t, map[string]http.HandlerFunc{ "POST /api/v1/commands/cancel": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") w.WriteHeader(http.StatusAccepted) _, _ = fmt.Fprint(w, `{"CommandIds":["cmd-1"]}`) }, @@ -343,6 +415,9 @@ func TestCancelCommands(t *testing.T) { if result.IsError { t.Fatalf("expected success, got error: %s", textContent(t, result)) } + if gotClientID != wantID { + t.Fatalf("Client-ID header = %q, want %q", gotClientID, wantID) + } } func TestForceSync(t *testing.T) { @@ -517,8 +592,11 @@ func TestApplyForma_InvalidMode(t *testing.T) { } func TestApplyForma_JSONFile(t *testing.T) { + wantID := withClientIDFile(t) + var gotClientID string agent := mockAgent(t, map[string]http.HandlerFunc{ "POST /api/v1/commands": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") w.WriteHeader(http.StatusAccepted) _, _ = fmt.Fprint(w, `{"id":"cmd-apply-1","status":"pending"}`) }, @@ -546,6 +624,9 @@ func TestApplyForma_JSONFile(t *testing.T) { if result.IsError { t.Fatalf("expected success, got error: %s", textContent(t, result)) } + if gotClientID != wantID { + t.Fatalf("Client-ID header = %q, want %q", gotClientID, wantID) + } } func TestDestroyForma_MissingBothInputs(t *testing.T) { @@ -579,8 +660,11 @@ func TestDestroyForma_BothInputsProvided(t *testing.T) { } func TestDestroyForma_ByQuery(t *testing.T) { + wantID := withClientIDFile(t) + var gotClientID string agent := mockAgent(t, map[string]http.HandlerFunc{ "POST /api/v1/commands": func(w http.ResponseWriter, r *http.Request) { + gotClientID = r.Header.Get("Client-ID") w.WriteHeader(http.StatusAccepted) _, _ = fmt.Fprint(w, `{"id":"cmd-destroy-1","status":"pending"}`) }, @@ -601,6 +685,9 @@ func TestDestroyForma_ByQuery(t *testing.T) { if result.IsError { t.Fatalf("expected success, got error: %s", textContent(t, result)) } + if gotClientID != wantID { + t.Fatalf("Client-ID header = %q, want %q", gotClientID, wantID) + } } // --- Changes since last reconcile tests --- diff --git a/internal/tools/descriptions.go b/internal/tools/descriptions.go index bd014ad..cd0ac1c 100644 --- a/internal/tools/descriptions.go +++ b/internal/tools/descriptions.go @@ -46,7 +46,8 @@ Use this tool when the user asks about running commands, recent deployments, com Query syntax uses field:value pairs. Supported fields: - id: filter by command ID -- client: filter by client ('client:me' for this session's commands) +- client: filter by client ('client:me' resolves to this machine's formae client id, shared with the CLI when its id file exists, else the shared 'formae-mcp' fallback; not the human) +- user: filter by user ('user:me' for the human behind the request's bearer token; a UUID matches the subject id, anything else matches the display name) - command: filter by type ('command:apply' or 'command:destroy') - status: filter by state ('status:in_progress', 'status:completed', 'status:failed') - stack: filter by stack name diff --git a/internal/tools/types.go b/internal/tools/types.go index 71d0597..1b2e771 100644 --- a/internal/tools/types.go +++ b/internal/tools/types.go @@ -29,7 +29,7 @@ type GetCommandStatusInput struct { // ListCommandsInput is the input for the list_commands tool. type ListCommandsInput struct { - Query string `json:"query,omitempty" jsonschema:"Query to filter commands. Supported fields: id, client, command (apply/destroy), status (pending/in_progress/completed/failed), stack, managed. Use 'client:me' to filter to your own commands. Leave empty for most recent commands."` + Query string `json:"query,omitempty" jsonschema:"Query to filter commands. Supported fields: id, client, user, command (apply/destroy), status (pending/in_progress/completed/failed), stack, managed. Use 'client:me' for this session's own commands (this machine's formae client id, shared with the CLI when its id file exists, else the shared 'formae-mcp' fallback; not the human) or 'user:me' for the human behind the bearer token; a user UUID matches the subject id, anything else matches the display name. Leave empty for most recent commands."` MaxResults string `json:"max_results,omitempty" jsonschema:"Maximum number of commands to return. Defaults to 10."` Profile string `json:"profile,omitempty" jsonschema:"Preferred way to target a named formae environment/agent for THIS call only, without changing global state. Use this in preference to use_profile for per-session targeting: the active profile is global and shared with the user's CLI and any other concurrent sessions, so switching it can hijack work elsewhere. Leave empty to use the active profile. See list_profiles for names. Requires formae >= 0.87.0."` }