diff --git a/frontend/src/types/api.ts b/frontend/src/types/api.ts index bf54f868..3f41539d 100644 --- a/frontend/src/types/api.ts +++ b/frontend/src/types/api.ts @@ -883,6 +883,9 @@ export interface ClientStatus { // privacy fix including the exact tccutil reset command). access_state?: AccessState remediation?: string + // Every config location the existence check consults, highest precedence + // first (e.g. OpenCode's opencode.jsonc then opencode.json). + checked_paths?: string[] } export interface ConnectResult { diff --git a/internal/connect/checked_paths_test.go b/internal/connect/checked_paths_test.go new file mode 100644 index 00000000..26f8bd7f --- /dev/null +++ b/internal/connect/checked_paths_test.go @@ -0,0 +1,100 @@ +package connect + +import ( + "os" + "path/filepath" + "runtime" + "testing" +) + +// A "No config found" row must be able to name the exact files that were +// looked for — otherwise a user whose opencode.jsonc exists cannot tell +// whether mcpproxy checked the wrong file or could not see it. + +func statusFor(t *testing.T, statuses []ClientStatus, id string) ClientStatus { + t.Helper() + for _, st := range statuses { + if st.ID == id { + return st + } + } + t.Fatalf("client %q not in status list", id) + return ClientStatus{} +} + +func TestGetAllStatusReportsCheckedPaths(t *testing.T) { + if runtime.GOOS == "windows" { + t.Setenv("LOCALAPPDATA", "") + } + home := t.TempDir() + s := NewServiceWithHome("127.0.0.1:8080", "key", home) + statuses := s.GetAllStatus() + + // OpenCode consults both candidates, .jsonc first (it shadows .json), even + // when neither exists — the row must say so instead of naming only the + // create-new default. + oc := statusFor(t, statuses, "opencode") + want := opencodeConfigCandidates(home) + if len(oc.CheckedPaths) != len(want) { + t.Fatalf("opencode checked_paths = %v, want %v", oc.CheckedPaths, want) + } + for i := range want { + if oc.CheckedPaths[i] != want[i] { + t.Fatalf("opencode checked_paths[%d] = %s, want %s", i, oc.CheckedPaths[i], want[i]) + } + } + if filepath.Base(oc.CheckedPaths[0]) != "opencode.jsonc" { + t.Fatalf("opencode checked_paths[0] = %s, want opencode.jsonc first", oc.CheckedPaths[0]) + } + + // A single-file client reports exactly its one path. + cursor := statusFor(t, statuses, "cursor") + if len(cursor.CheckedPaths) != 1 || cursor.CheckedPaths[0] != ConfigPath("cursor", home) { + t.Fatalf("cursor checked_paths = %v, want [%s]", cursor.CheckedPaths, ConfigPath("cursor", home)) + } +} + +// Production Services are built without a homeDir (NewService); the opencode +// candidates must still resolve to absolute paths, or both the jsonc +// preference and checked_paths silently degrade to CWD-relative stats. +func TestOpencodePathsAreAbsoluteWithoutAnExplicitHome(t *testing.T) { + if _, err := os.UserHomeDir(); err != nil { + t.Skip("no resolvable home directory") + } + s := NewService("127.0.0.1:8080", "key") + + paths := s.checkedPaths("opencode") + if len(paths) != 2 { + t.Fatalf("checked paths = %v, want both opencode candidates", paths) + } + for _, p := range paths { + if !filepath.IsAbs(p) { + t.Fatalf("checked path %q is CWD-relative — empty homeDir must resolve via os.UserHomeDir", p) + } + } + if p := s.configPath("opencode"); !filepath.IsAbs(p) { + t.Fatalf("configPath %q is CWD-relative — the jsonc preference never fires in production", p) + } +} + +func TestGetStatusReportsCheckedPaths(t *testing.T) { + if runtime.GOOS == "windows" { + t.Setenv("LOCALAPPDATA", "") + } + home := t.TempDir() + s := NewServiceWithHome("127.0.0.1:8080", "key", home) + + st, err := s.GetStatus("opencode") + if err != nil { + t.Fatal(err) + } + want := opencodeConfigCandidates(home) + if len(st.CheckedPaths) != len(want) { + t.Fatalf("opencode checked_paths = %v, want %v", st.CheckedPaths, want) + } + for i := range want { + if st.CheckedPaths[i] != want[i] { + t.Fatalf("opencode checked_paths[%d] = %s, want %s", i, st.CheckedPaths[i], want[i]) + } + } +} diff --git a/internal/connect/clients.go b/internal/connect/clients.go index 9c041179..82d1fbb2 100644 --- a/internal/connect/clients.go +++ b/internal/connect/clients.go @@ -188,7 +188,18 @@ func opencodeConfigDir(homeDir string) string { // opencodeConfigCandidates lists the global config files OpenCode itself loads, // highest precedence first: opencode.jsonc shadows opencode.json for the same // keys, and recent OpenCode versions bootstrap the .jsonc variant (#922). +// +// An empty homeDir resolves through os.UserHomeDir, exactly like ConfigPath — +// production Services are built without one (NewService), and joining "" would +// yield CWD-relative candidates that stat against the wrong directory. func opencodeConfigCandidates(homeDir string) []string { + if homeDir == "" { + var err error + homeDir, err = os.UserHomeDir() + if err != nil { + return nil + } + } dir := opencodeConfigDir(homeDir) return []string{ filepath.Join(dir, "opencode.jsonc"), @@ -217,6 +228,19 @@ func (s *Service) configPath(clientID string) string { return ConfigPath(clientID, s.homeDir) } +// checkedPaths lists the config files the existence check consults for a +// client, highest precedence first — the paths a "no config found" UI should +// name. Static: no stat calls, so it is safe on the content-read-free path. +func (s *Service) checkedPaths(clientID string) []string { + if clientID == "opencode" { + return opencodeConfigCandidates(s.homeDir) + } + if p := ConfigPath(clientID, s.homeDir); p != "" { + return []string{p} + } + return nil +} + // buildServerEntry returns the JSON/TOML-serializable map inserted into the // client's config for the mcpproxy endpoint. When p.credential is set (only when // require_mcp_auth is on), it is written via the carrier each client actually diff --git a/internal/connect/connect.go b/internal/connect/connect.go index fff0898a..7a5f6db6 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -56,6 +56,11 @@ type ClientStatus struct { // Empty/"unknown" in the content-read-free overall status; resolved to // "accessible"/"absent"/"malformed" (and "denied" in US2) by on-demand reads. AccessState string `json:"access_state"` + // CheckedPaths lists every config location the existence check consults, + // highest precedence first. For most clients this is just ConfigPath; for + // OpenCode it names both opencode.jsonc and opencode.json (#922), so a + // "no config found" UI can say exactly which files were looked for. + CheckedPaths []string `json:"checked_paths,omitempty"` // Remediation carries actionable fix text, populated only when access is denied. Remediation string `json:"remediation,omitempty"` } @@ -298,15 +303,16 @@ func (s *Service) GetAllStatus() []ClientStatus { for _, c := range clients { cfgPath := s.configPath(c.ID) status := ClientStatus{ - ID: c.ID, - Name: c.Name, - ConfigPath: cfgPath, - Supported: c.Supported, - Reason: c.Reason, - Note: c.Note, - Bridge: c.Bridge, - Icon: c.Icon, - AccessState: accessUnknown, + ID: c.ID, + Name: c.Name, + ConfigPath: cfgPath, + CheckedPaths: s.checkedPaths(c.ID), + Supported: c.Supported, + Reason: c.Reason, + Note: c.Note, + Bridge: c.Bridge, + Icon: c.Icon, + AccessState: accessUnknown, } // Metadata-only existence check (no content read). @@ -338,15 +344,16 @@ func (s *Service) GetStatus(clientID string) (ClientStatus, error) { cfgPath := s.configPath(c.ID) status := ClientStatus{ - ID: c.ID, - Name: c.Name, - ConfigPath: cfgPath, - Supported: c.Supported, - Reason: c.Reason, - Note: c.Note, - Bridge: c.Bridge, - Icon: c.Icon, - AccessState: accessUnknown, + ID: c.ID, + Name: c.Name, + ConfigPath: cfgPath, + CheckedPaths: s.checkedPaths(c.ID), + Supported: c.Supported, + Reason: c.Reason, + Note: c.Note, + Bridge: c.Bridge, + Icon: c.Icon, + AccessState: accessUnknown, } if _, err := s.stat(cfgPath); err == nil { diff --git a/native/macos/MCPProxy/MCPProxy/API/APIClient.swift b/native/macos/MCPProxy/MCPProxy/API/APIClient.swift index 03ea201b..04b31aec 100644 --- a/native/macos/MCPProxy/MCPProxy/API/APIClient.swift +++ b/native/macos/MCPProxy/MCPProxy/API/APIClient.swift @@ -313,6 +313,9 @@ actor APIClient { let note: String? /// Connects through a stdio bridge; connectable without an existing config. let bridge: Bool? + /// Every config location the core's existence check consults, highest + /// precedence first (e.g. OpenCode's opencode.jsonc then opencode.json). + let checkedPaths: [String]? enum CodingKeys: String, CodingKey { case clientId = "id" @@ -322,6 +325,7 @@ actor APIClient { case serverName = "server_name" case accessState = "access_state" case remediation + case checkedPaths = "checked_paths" } /// Name to render; a core newer than the app may report a client this diff --git a/native/macos/MCPProxy/MCPProxy/Views/ConnectClientModel.swift b/native/macos/MCPProxy/MCPProxy/Views/ConnectClientModel.swift index cc931db5..e3ea51b5 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ConnectClientModel.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ConnectClientModel.swift @@ -208,6 +208,11 @@ final class ConnectClientModel: ObservableObject { /// Extra guidance for the row: the core's remediation when access is /// denied, or its caveat for a supported client. let note: String? + /// Whether the note is cautionary (remediation, a core caveat) or + /// merely informational (the looked-for paths). Drives which visual + /// channel renders it — the warning channel must not be diluted by + /// path hints on every not-installed row. + let noteIsWarning: Bool let connected: Bool var id: String { clientId } @@ -387,13 +392,15 @@ final class ConnectClientModel: ObservableObject { private func row(for client: APIClient.ClientStatus) -> ClientRow { let resolved = resolvedDetails[client.clientId] ?? client + let note = Self.note(for: resolved) return ClientRow( clientId: client.clientId, displayName: client.displayName, symbolName: client.symbolName, stateLabel: Self.stateLabel(for: resolved), isSelectable: client.supported, - note: Self.note(for: resolved), + note: note?.text, + noteIsWarning: note?.isWarning ?? false, connected: resolved.connected ) } @@ -422,15 +429,51 @@ final class ConnectClientModel: ObservableObject { return client.exists ? "Config present" : "No config found" } - private static func note(for client: APIClient.ClientStatus) -> String? { + private static func note(for client: APIClient.ClientStatus) -> (text: String, isWarning: Bool)? { if client.accessState == .denied, let remediation = client.remediation, !remediation.isEmpty { - return remediation + return (remediation, true) + } + if let note = client.note, !note.isEmpty { return (note, true) } + if client.supported, !client.connected, !client.exists { + switch client.accessState { + case .none, .unknown, .absent: + if let looked = lookedForDescription(for: client) { return (looked, false) } + case .accessible, .denied, .malformed: + // A denied stat is not evidence of absence: the aggregate list + // sets "denied" WITHOUT remediation, and saying "Looked for …" + // there would claim the files were checked when the check was + // forbidden — the exact wrong-problem label the core refuses + // to produce. + break + } } - if let note = client.note, !note.isEmpty { return note } return nil } + /// Names the exact files behind a "No config found" verdict, so a user + /// whose config lives at a path this build never checks (say, an + /// opencode.jsonc against a pre-#923 core) can see the mismatch instead of + /// guessing. + private static func lookedForDescription(for client: APIClient.ClientStatus) -> String? { + var paths = client.checkedPaths ?? [] + if paths.isEmpty, !client.configPath.isEmpty { paths = [client.configPath] } + guard !paths.isEmpty else { return nil } + let abbreviated = paths.map(abbreviatingHome) + let dirs = Set(abbreviated.map { ($0 as NSString).deletingLastPathComponent }) + if abbreviated.count > 1, dirs.count == 1, let dir = dirs.first, !dir.isEmpty { + let names = abbreviated.map { ($0 as NSString).lastPathComponent } + return "Looked for \(names.joined(separator: " or ")) in \(dir)" + } + return "Looked for \(abbreviated.joined(separator: ", "))" + } + + private static func abbreviatingHome(_ path: String) -> String { + let home = NSHomeDirectory() + guard home.count > 1, path.hasPrefix(home + "/") else { return path } + return "~" + path.dropFirst(home.count) + } + // MARK: - Derived /// The currently rendered preview, or nil when none is bound to the current diff --git a/native/macos/MCPProxy/MCPProxy/Views/ConnectClientView.swift b/native/macos/MCPProxy/MCPProxy/Views/ConnectClientView.swift index 424c0bbf..c3394745 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/ConnectClientView.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/ConnectClientView.swift @@ -231,7 +231,7 @@ struct ConnectClientView: View { if let note = row.note { Text(note) .font(.caption2) - .foregroundStyle(.orange) + .foregroundStyle(row.noteIsWarning ? Color.orange : Color.secondary) } } Spacer() @@ -239,7 +239,9 @@ struct ConnectClientView: View { .padding(.vertical, 2) .opacity(row.isSelectable ? 1 : 0.5) .accessibilityElement(children: .combine) - .accessibilityLabel("\(row.displayName), \(row.stateLabel)") + .accessibilityLabel( + row.note.map { "\(row.displayName), \(row.stateLabel), \($0)" } + ?? "\(row.displayName), \(row.stateLabel)") } // MARK: Detail + preview diff --git a/native/macos/MCPProxy/MCPProxyTests/ConnectClientModelTests.swift b/native/macos/MCPProxy/MCPProxyTests/ConnectClientModelTests.swift index b8ec05b2..193d915f 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ConnectClientModelTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ConnectClientModelTests.swift @@ -477,6 +477,103 @@ final class ConnectClientModelTests: XCTestCase { XCTAssertTrue(source.previewCalls.isEmpty) } + /// "No config found" must name the files the verdict is about: a user whose + /// opencode.jsonc exists needs to see which paths were actually checked + /// before concluding the client is not set up. + func testANoConfigRowNamesTheCheckedPaths() async { + let source = FakeConnectSource() + source.clientsResults = [.success([ + FakeConnectSource.client( + id: "opencode", name: "OpenCode", exists: false, + checkedPaths: ["/Users/x/.config/opencode/opencode.jsonc", + "/Users/x/.config/opencode/opencode.json"]), + FakeConnectSource.client(id: "cursor", name: "Cursor", exists: false), + FakeConnectSource.client(id: "claude-code", name: "Claude Code", exists: true) + ])] + let model = makeModel(source) + + await model.loadList() + + // Same directory: name the files once and the directory once. + XCTAssertEqual( + model.rows[0].note, + "Looked for opencode.jsonc or opencode.json in /Users/x/.config/opencode") + // A core without checked_paths still names its single config_path. + XCTAssertEqual(model.rows[1].note, "Looked for /Users/x/.cursor/config.json") + // A present config needs no explanation of where it was looked for. + XCTAssertNil(model.rows[2].note) + } + + /// The looked-for hint never displaces a real caveat: an explicit core note + /// (e.g. a bridge requirement) outranks it — and keeps the warning channel. + func testACoreNoteOutranksTheLookedForHint() async { + let source = FakeConnectSource() + source.clientsResults = [.success([ + FakeConnectSource.client(id: "claude-desktop", exists: false, + note: "Requires the bundled stdio bridge") + ])] + let model = makeModel(source) + + await model.loadList() + + XCTAssertEqual(model.rows.first?.note, "Requires the bundled stdio bridge") + XCTAssertEqual(model.rows.first?.noteIsWarning, true) + } + + /// A denied stat is not evidence of absence. The aggregate list classifies a + /// permission-blocked stat as denied WITHOUT remediation; that row must not + /// say "Looked for …" — the files were never checked, and the note would + /// name the wrong problem under an "Access not granted" label. + func testADeniedRowWithoutRemediationGetsNoLookedForNote() async { + let source = FakeConnectSource() + source.clientsResults = [.success([ + FakeConnectSource.client(id: "cursor", exists: false, accessState: .denied) + ])] + let model = makeModel(source) + + await model.loadList() + + XCTAssertEqual(model.rows.first?.stateLabel, "Access not granted") + XCTAssertNil(model.rows.first?.note) + } + + /// The looked-for hint is informational, not cautionary — it must not light + /// up the warning channel every real remediation uses. + func testTheLookedForHintIsNotAWarning() async { + let source = FakeConnectSource() + source.clientsResults = [.success([ + FakeConnectSource.client(id: "cursor", exists: false) + ])] + let model = makeModel(source) + + await model.loadList() + + XCTAssertEqual(model.rows.first?.noteIsWarning, false) + } + + /// Paths under the actual home directory — which is where every real client + /// config lives — render tilde-abbreviated, with the shared directory named + /// once. + func testCheckedPathsUnderTheRealHomeAreTildeAbbreviated() async { + let home = NSHomeDirectory() + let source = FakeConnectSource() + source.clientsResults = [.success([ + FakeConnectSource.client( + id: "opencode", exists: false, + checkedPaths: ["\(home)/.config/opencode/opencode.jsonc", + "\(home)/.config/opencode/opencode.json"]), + FakeConnectSource.client(id: "cursor", exists: false, + checkedPaths: ["\(home)/.cursor/mcp.json"]) + ])] + let model = makeModel(source) + + await model.loadList() + + XCTAssertEqual(model.rows[0].note, + "Looked for opencode.jsonc or opencode.json in ~/.config/opencode") + XCTAssertEqual(model.rows[1].note, "Looked for ~/.cursor/mcp.json") + } + /// An unsupported client the core gave no reason for still renders disabled /// with a defined label rather than an empty one. func testAnUnsupportedRowWithoutAReasonStillCarriesALabel() async { diff --git a/native/macos/MCPProxy/MCPProxyTests/Support/FakeConnectSource.swift b/native/macos/MCPProxy/MCPProxyTests/Support/FakeConnectSource.swift index 34332ab8..8fdf9863 100644 --- a/native/macos/MCPProxy/MCPProxyTests/Support/FakeConnectSource.swift +++ b/native/macos/MCPProxy/MCPProxyTests/Support/FakeConnectSource.swift @@ -122,7 +122,9 @@ final class FakeConnectSource: ConnectClientDataSource, @unchecked Sendable { reason: String? = nil, accessState: ConnectAccessState? = nil, remediation: String? = nil, - serverName: String? = nil + serverName: String? = nil, + note: String? = nil, + checkedPaths: [String]? = nil ) -> APIClient.ClientStatus { let json: [String: Any?] = [ "id": id, @@ -135,7 +137,9 @@ final class FakeConnectSource: ConnectClientDataSource, @unchecked Sendable { "icon": id, "server_name": serverName, "access_state": accessState?.rawValue, - "remediation": remediation + "remediation": remediation, + "note": note, + "checked_paths": checkedPaths ] let data = try! JSONSerialization.data( withJSONObject: json.compactMapValues { $0 })