From ecaf3407fa52fee5c9e8c30569900fd2b8757451 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 01:57:18 +0400 Subject: [PATCH 1/2] media store: name the constants and fold the shared write path (V-581) Put and PutFile were the same function twice: cap check, path build, blob description, budget claim, sidecar first. Extract reserve, release, bucket and newBlob so each caller reads as its own difference. List and pruneOrphans walked a kind's tree with the same skeleton, so that is now walkSidecars and walkBlobFiles over one walkKind. Name what was a literal in five places: 0o700, 0o600, ".json", "spool", the two-character bucket and the 64-character id. The kind list is one var rather than four copies. Rename the stat result in both writers. It was called "already", and "already != nil" meant the blob was NOT already there, which reads backwards at every use. It is now statErr with a fresh bool beside it. No behaviour change. The reservation leak on a failed writeMeta is preserved on purpose and filed as V-584. Co-Authored-By: Claude Opus 5 --- internal/media/store.go | 250 +++++++++++++++++++++++----------------- 1 file changed, 146 insertions(+), 104 deletions(-) diff --git a/internal/media/store.go b/internal/media/store.go index 32d3fc4..b931e9f 100644 --- a/internal/media/store.go +++ b/internal/media/store.go @@ -42,6 +42,29 @@ const DefaultRetention = 7 * 24 * time.Hour // thousand photos inside the window. const DefaultMaxTotalBytes int64 = 4 << 30 +const ( + // dirPerm and filePerm: these are recordings of people, so the daemon's + // user is the only reader. + dirPerm fs.FileMode = 0o700 + filePerm fs.FileMode = 0o600 + // metaExt is the sidecar suffix. Anything else under a bucket is blob + // bytes, which is how the walkers tell the two apart. + metaExt = ".json" + // spoolName is the incremental-write directory, held outside the kind + // directories so no walker mistakes a half-written file for a blob. + spoolName = "spool" + // bucketPrefix is how many leading id characters name the subdirectory, so + // one kind is spread over 256 directories rather than one flat listing. + bucketPrefix = 2 + // idLen is the length of a hex sha256, which is the only id shape a path + // is ever built from. + idLen = 64 +) + +// allKinds is every kind a walker has to visit. A store-wide operation covers +// all of them, and this is the one list to extend when a third kind lands. +var allKinds = []Kind{KindImage, KindAudio} + // ErrStoreFull — the store is at its total-bytes budget. Distinct from // ErrTooLarge: the payload is a reasonable size and there is no room for it, so // the answer is to prune or raise the budget, not to send something smaller. @@ -85,7 +108,7 @@ func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duratio if err != nil { return nil, fmt.Errorf("media: resolve dir: %w", err) } - if err := os.MkdirAll(abs, 0o700); err != nil { + if err := os.MkdirAll(abs, dirPerm); err != nil { return nil, fmt.Errorf("media: create dir: %w", err) } if maxBytes <= 0 { @@ -117,14 +140,14 @@ func OpenWithBudget(dir string, maxBytes, maxTotal int64, retention time.Duratio // over at zero. func (s *Store) measure() int64 { var total int64 - spool := filepath.Join(s.dir, "spool") + spool := filepath.Join(s.dir, spoolName) _ = filepath.WalkDir(s.dir, func(path string, d fs.DirEntry, err error) error { if err == nil && d.IsDir() && path == spool { // Spool files are not blobs yet and PutFile counts them when they // become one. Counting them here too would double them. return filepath.SkipDir } - if err != nil || d.IsDir() || strings.HasSuffix(path, ".json") { + if err != nil || d.IsDir() || strings.HasSuffix(path, metaExt) { return nil //nolint:nilerr // an unreadable corner is not worth refusing to boot over } if info, err := d.Info(); err == nil { @@ -153,6 +176,54 @@ func (s *Store) Dir() string { return s.dir } // Retention is the configured age limit Prune enforces. func (s *Store) Retention() time.Duration { return s.retention } +// reserve claims size against the whole-store budget and reports ErrStoreFull +// when there is no room. Claiming before the write means two concurrent Puts +// cannot both pass a check that only one of them fits through. +func (s *Store) reserve(size int64) error { + s.totalMu.Lock() + room := s.total+size <= s.maxTotal + if room { + s.total += size + } + stored := s.total + s.totalMu.Unlock() + if !room { + return fmt.Errorf("%w: %d stored, %d budget, %d more asked for", + ErrStoreFull, stored, s.maxTotal, size) + } + return nil +} + +// release gives size back, for a reservation whose write failed and for bytes +// a delete removed. The floor at zero keeps a miscount from reading as a store +// that owes itself space. +func (s *Store) release(size int64) { + s.totalMu.Lock() + s.total -= size + if s.total < 0 { + s.total = 0 + } + s.totalMu.Unlock() +} + +// bucket is the directory an id lives in. Every path the store builds goes +// through here, so the traversal guard in validID has one place to sit. +func (s *Store) bucket(kind Kind, id string) string { + return filepath.Join(s.dir, string(kind), id[:bucketPrefix]) +} + +// newBlob describes what is about to be stored. A blob already on disk keeps +// its first-seen time: re-sending the same photo every hour must not keep it +// alive past retention. +func (s *Store) newBlob(kind Kind, mime, source, id, blobPath, metaPath string, size int64) Blob { + b := Blob{ID: id, Kind: kind, MIME: mime, Size: size, Source: source, + Created: s.now().UTC(), Path: blobPath} + if prev, err := readMeta(metaPath); err == nil && !prev.Created.IsZero() { + b.Created = prev.Created + } + return b +} + // Put stores data and returns its Blob. The id is the sha256 of data, so // storing the same bytes twice is idempotent: the second call rewrites the // sidecar (keeping the ORIGINAL creation time, so a re-send cannot extend @@ -178,32 +249,19 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { if err != nil { return Blob{}, err } - if err := os.MkdirAll(filepath.Dir(blobPath), 0o700); err != nil { + if err := os.MkdirAll(filepath.Dir(blobPath), dirPerm); err != nil { return Blob{}, fmt.Errorf("media: create bucket: %w", err) } - b := Blob{ID: id, Kind: kind, MIME: mime, Size: int64(len(data)), Source: source, - Created: s.now().UTC(), Path: blobPath} - - // A blob already here keeps its first-seen time. Re-sending the same photo - // every hour must not keep it alive past retention. - if prev, err := readMeta(metaPath); err == nil && !prev.Created.IsZero() { - b.Created = prev.Created - } + b := s.newBlob(kind, mime, source, id, blobPath, metaPath, int64(len(data))) // A blob already on disk costs nothing more, so dedupe is checked before // the budget rather than after it. - _, already := os.Stat(blobPath) - if already != nil { - s.totalMu.Lock() - room := s.total+b.Size <= s.maxTotal - if room { - s.total += b.Size - } - s.totalMu.Unlock() - if !room { - return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", - ErrStoreFull, s.Total(), s.maxTotal, b.Size) + _, statErr := os.Stat(blobPath) + fresh := statErr != nil + if fresh { + if err := s.reserve(b.Size); err != nil { + return Blob{}, err } } @@ -216,10 +274,8 @@ func (s *Store) Put(kind Kind, mime, source string, data []byte) (Blob, error) { } if err := writeFile(blobPath, data); err != nil { _ = os.Remove(metaPath) - if already != nil { - s.totalMu.Lock() - s.total -= b.Size - s.totalMu.Unlock() + if fresh { + s.release(b.Size) } return Blob{}, err } @@ -264,43 +320,31 @@ func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { if err != nil { return Blob{}, err } - if err := os.MkdirAll(filepath.Dir(blobPath), 0o700); err != nil { + if err := os.MkdirAll(filepath.Dir(blobPath), dirPerm); err != nil { return Blob{}, fmt.Errorf("media: create bucket: %w", err) } - b := Blob{ID: id, Kind: kind, MIME: mime, Size: info.Size(), Source: source, - Created: s.now().UTC(), Path: blobPath} - if prev, err := readMeta(metaPath); err == nil && !prev.Created.IsZero() { - b.Created = prev.Created - } - _, already := os.Stat(blobPath) - if already != nil { - s.totalMu.Lock() - room := s.total+b.Size <= s.maxTotal - if room { - s.total += b.Size - } - s.totalMu.Unlock() - if !room { - return Blob{}, fmt.Errorf("%w: %d stored, %d budget, %d more asked for", - ErrStoreFull, s.Total(), s.maxTotal, b.Size) + b := s.newBlob(kind, mime, source, id, blobPath, metaPath, info.Size()) + _, statErr := os.Stat(blobPath) + fresh := statErr != nil + if fresh { + if err := s.reserve(b.Size); err != nil { + return Blob{}, err } } if err := writeMeta(metaPath, b); err != nil { return Blob{}, err } - if already == nil { + if !fresh { // Same bytes already here. Drop the spool copy. _ = os.Remove(src) return b, nil } - if err := os.Chmod(src, 0o600); err != nil { + if err := os.Chmod(src, filePerm); err != nil { return Blob{}, fmt.Errorf("media: chmod spool: %w", err) } if err := os.Rename(src, blobPath); err != nil { _ = os.Remove(metaPath) - s.totalMu.Lock() - s.total -= b.Size - s.totalMu.Unlock() + s.release(b.Size) return Blob{}, fmt.Errorf("media: move spool: %w", err) } return b, nil @@ -311,15 +355,15 @@ func (s *Store) PutFile(kind Kind, mime, source, src string) (Blob, error) { // looks at it and List never reports it; PutFile is what turns it into a blob. // The caller owns removing it if it never gets that far. func (s *Store) SpoolFile(prefix string) (*os.File, error) { - dir := filepath.Join(s.dir, "spool") - if err := os.MkdirAll(dir, 0o700); err != nil { + dir := filepath.Join(s.dir, spoolName) + if err := os.MkdirAll(dir, dirPerm); err != nil { return nil, fmt.Errorf("media: create spool: %w", err) } f, err := os.CreateTemp(dir, prefix+"-*") if err != nil { return nil, fmt.Errorf("media: spool: %w", err) } - if err := f.Chmod(0o600); err != nil { + if err := f.Chmod(filePerm); err != nil { f.Close() return nil, fmt.Errorf("media: chmod spool: %w", err) } @@ -346,9 +390,8 @@ func (s *Store) Get(id string) (Blob, error) { if !validID(id) { return Blob{}, ErrBadID } - for _, kind := range []Kind{KindImage, KindAudio} { - metaPath := filepath.Join(s.dir, string(kind), id[:2], id+".json") - b, err := readMeta(metaPath) + for _, kind := range allKinds { + b, err := readMeta(filepath.Join(s.bucket(kind, id), id+metaExt)) if err != nil { continue } @@ -382,7 +425,7 @@ func (s *Store) Read(id string) (Blob, []byte, error) { // inside the retention window) that is cheap, and it means the sidecars are the // single source of truth with no index to fall out of sync. func (s *Store) List(kind Kind) ([]Blob, error) { - kinds := []Kind{KindImage, KindAudio} + kinds := allKinds if kind != "" { if !kind.Valid() { return nil, ErrBadKind @@ -391,17 +434,7 @@ func (s *Store) List(kind Kind) ([]Blob, error) { } var out []Blob for _, k := range kinds { - root := filepath.Join(s.dir, string(k)) - err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil // kind never used; not an error - } - return err - } - if d.IsDir() || !strings.HasSuffix(path, ".json") { - return nil - } + err := s.walkSidecars(k, func(path string, _ fs.DirEntry) error { b, err := readMeta(path) if err != nil { return nil // a corrupt sidecar is skipped, not fatal @@ -431,8 +464,8 @@ func (s *Store) Delete(id string) error { if !validID(id) { return ErrBadID } - for _, kind := range []Kind{KindImage, KindAudio} { - bucket := filepath.Join(s.dir, string(kind), id[:2]) + for _, kind := range allKinds { + bucket := s.bucket(kind, id) entries, err := os.ReadDir(bucket) if err != nil { continue @@ -441,21 +474,17 @@ func (s *Store) Delete(id string) error { if !strings.HasPrefix(e.Name(), id) { continue } - path := filepath.Join(bucket, e.Name()) + // Only the bytes count against the budget, so the sidecar's own + // size is never given back. var size int64 - if info, err := e.Info(); err == nil && !strings.HasSuffix(e.Name(), ".json") { + if info, err := e.Info(); err == nil && !strings.HasSuffix(e.Name(), metaExt) { size = info.Size() } - if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { + if err := os.Remove(filepath.Join(bucket, e.Name())); err != nil && !errors.Is(err, fs.ErrNotExist) { return fmt.Errorf("media: delete %s: %w", shortID(id), err) } if size > 0 { - s.totalMu.Lock() - s.total -= size - if s.total < 0 { - s.total = 0 - } - s.totalMu.Unlock() + s.release(size) } } } @@ -498,20 +527,9 @@ func (s *Store) Prune() (int, error) { // Put racing a Prune does not lose its bytes. func (s *Store) pruneOrphans(known map[string]bool, now time.Time) (int, error) { deleted := 0 - for _, kind := range []Kind{KindImage, KindAudio} { - root := filepath.Join(s.dir, string(kind)) - err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { - if err != nil { - if errors.Is(err, fs.ErrNotExist) { - return nil - } - return err - } - if d.IsDir() || strings.HasSuffix(path, ".json") { - return nil - } - name := d.Name() - id, _, _ := strings.Cut(name, ".") + for _, kind := range allKinds { + err := s.walkBlobFiles(kind, func(path string, d fs.DirEntry) error { + id, _, _ := strings.Cut(d.Name(), ".") if known[id] { return nil } @@ -525,12 +543,7 @@ func (s *Store) pruneOrphans(known map[string]bool, now time.Time) (int, error) if err := os.Remove(path); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } - s.totalMu.Lock() - s.total -= info.Size() - if s.total < 0 { - s.total = 0 - } - s.totalMu.Unlock() + s.release(info.Size()) deleted++ return nil }) @@ -541,13 +554,42 @@ func (s *Store) pruneOrphans(known map[string]bool, now time.Time) (int, error) return deleted, nil } +// walkSidecars visits every sidecar of one kind, and walkBlobFiles every file +// that is not one. The sidecars are the store's index and the rest are the +// bytes, so a walker always wants exactly one of the two. +func (s *Store) walkSidecars(kind Kind, fn func(path string, d fs.DirEntry) error) error { + return s.walkKind(kind, true, fn) +} + +func (s *Store) walkBlobFiles(kind Kind, fn func(path string, d fs.DirEntry) error) error { + return s.walkKind(kind, false, fn) +} + +// walkKind walks one kind's directory tree. A kind that was never used has no +// directory, which is silence rather than an error. +func (s *Store) walkKind(kind Kind, sidecars bool, fn func(path string, d fs.DirEntry) error) error { + root := filepath.Join(s.dir, string(kind)) + return filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return nil + } + return err + } + if d.IsDir() || strings.HasSuffix(path, metaExt) != sidecars { + return nil + } + return fn(path, d) + }) +} + // paths returns the blob and sidecar paths for an id. func (s *Store) paths(kind Kind, id, mime string) (blobPath, metaPath string, err error) { if !validID(id) { return "", "", ErrBadID } - bucket := filepath.Join(s.dir, string(kind), id[:2]) - return filepath.Join(bucket, id+extFor(mime, kind)), filepath.Join(bucket, id+".json"), nil + bucket := s.bucket(kind, id) + return filepath.Join(bucket, id+extFor(mime, kind)), filepath.Join(bucket, id+metaExt), nil } // locate finds the stored bytes for an id whose extension we do not know, @@ -556,14 +598,14 @@ func (s *Store) locate(kind Kind, id string) (string, error) { if !validID(id) { return "", ErrBadID } - bucket := filepath.Join(s.dir, string(kind), id[:2]) + bucket := s.bucket(kind, id) entries, err := os.ReadDir(bucket) if err != nil { return "", ErrNotFound } for _, e := range entries { name := e.Name() - if strings.HasPrefix(name, id) && !strings.HasSuffix(name, ".json") { + if strings.HasPrefix(name, id) && !strings.HasSuffix(name, metaExt) { return filepath.Join(bucket, name), nil } } @@ -573,7 +615,7 @@ func (s *Store) locate(kind Kind, id string) (string, error) { // validID guards every path built from an id. Without it a caller-supplied id // is a path traversal: Get("../../etc/passwd") would read outside the store. func validID(id string) bool { - if len(id) != 64 { + if len(id) != idLen { return false } for i := 0; i < len(id); i++ { @@ -621,7 +663,7 @@ func writeFile(path string, data []byte) error { return fmt.Errorf("media: temp: %w", err) } defer os.Remove(tmp.Name()) - if err := tmp.Chmod(0o600); err != nil { + if err := tmp.Chmod(filePerm); err != nil { tmp.Close() return fmt.Errorf("media: chmod: %w", err) } From 439ceb5d8ed3a3325332ad8bd2a1d5e00134bdea Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 01:57:27 +0400 Subject: [PATCH 2/2] mcp manager: one lookup for the call paths, and split dial (V-581) Call, ReadResource and CallPositional each opened the connection map by hand, and Call took the mutex twice to answer one question. They now share lookup, which returns the client, the config and the tool in one critical section. dial did three things. Choosing the transport is openTransport, and recording a live connection is succeed, so the function reads as handshake then discovery. Also: argv is a method rather than an append repeated in dial and Status, the transport strings are constants, and the tail binding comes out of bindPositional as bindOne. The comment on filterTools claimed it drops nameless tools, which it never did. No behaviour change. Co-Authored-By: Claude Opus 5 --- internal/mcp/manager.go | 187 ++++++++++++++++++++++------------------ 1 file changed, 102 insertions(+), 85 deletions(-) diff --git a/internal/mcp/manager.go b/internal/mcp/manager.go index d8a7dc4..9c8c433 100644 --- a/internal/mcp/manager.go +++ b/internal/mcp/manager.go @@ -38,6 +38,12 @@ const ( DefaultMaxDescription = 400 ) +// The two transports, as Status reports them to the web surface. +const ( + transportStdio = "stdio" + transportHTTP = "http" +) + var ( // ErrNoServer — the named server is not configured. ErrNoServer = errors.New("mcp: no such server") @@ -90,6 +96,12 @@ type ServerConfig struct { Enabled bool `json:"enabled"` } +// argv is the stdio server's command line. It is argv and never a shell +// string, so the same slice serves both the exec and the /tools display. +func (c ServerConfig) argv() []string { + return append([]string{c.Command}, c.Args...) +} + // PosterFactory builds the HTTP door for one server. It is a factory rather // than a single shared Poster because allow_private is per server: the fetcher // that may reach http://localhost:9100/mcp must NOT be the same fetcher another @@ -230,18 +242,7 @@ func (m *Manager) dial(ctx context.Context, name string) error { c.lastTry = time.Now() m.mu.Unlock() - var tr transport - var err error - if cfg.Command != "" { - tr, err = newStdioTransport(ctx, append([]string{cfg.Command}, cfg.Args...), cfg.Env, cfg.Dir) - } else if m.newPoster == nil { - err = fmt.Errorf("server %q has a url but no http door was wired", name) - } else { - var poster Poster - if poster, err = m.newPoster(cfg); err == nil { - tr = newHTTPTransport(poster, cfg.URL, cfg.Headers) - } - } + tr, err := m.openTransport(ctx, cfg) if err != nil { m.fail(name, err) return err @@ -262,21 +263,44 @@ func (m *Manager) dial(ctx context.Context, name string) error { tools = nil } tools = filterTools(cfg, tools) - - m.mu.Lock() - if old := m.conns[name].client; old != nil { - _ = old.Close() - } - m.conns[name].client = cl - m.conns[name].tools = tools - m.conns[name].lastErr = nil - m.conns[name].fails = 0 - m.conns[name].dialedAt = time.Now() - m.mu.Unlock() + m.succeed(name, cl, tools) log.Printf("mcp: %s connected (%s %s), %d tool(s)", name, cl.Info().Name, cl.Info().Version, len(tools)) return nil } +// openTransport builds the door this server is configured for. A url server +// with no factory is one server's problem, reported here, so a bad block never +// stops the daemon. +func (m *Manager) openTransport(ctx context.Context, cfg ServerConfig) (transport, error) { + if cfg.Command != "" { + return newStdioTransport(ctx, cfg.argv(), cfg.Env, cfg.Dir) + } + if m.newPoster == nil { + return nil, fmt.Errorf("server %q has a url but no http door was wired", cfg.Name) + } + poster, err := m.newPoster(cfg) + if err != nil { + return nil, err + } + return newHTTPTransport(poster, cfg.URL, cfg.Headers), nil +} + +// succeed records a live connection and closes the one it replaces, so a +// re-dial does not leak the previous subprocess. +func (m *Manager) succeed(name string, cl *Client, tools []Tool) { + m.mu.Lock() + defer m.mu.Unlock() + c := m.conns[name] + if c == nil { + return + } + if c.client != nil { + _ = c.client.Close() + } + c.client, c.tools, c.lastErr, c.fails = cl, tools, nil, 0 + c.dialedAt = time.Now() +} + func (m *Manager) fail(name string, err error) { m.mu.Lock() defer m.mu.Unlock() @@ -288,8 +312,7 @@ func (m *Manager) fail(name string, err error) { } } -// filterTools applies AllowTools and MaxTools, drops nameless entries and -// truncates descriptions. +// filterTools applies AllowTools and MaxTools and truncates descriptions. // // Over the cap WITHOUT allow_tools, the whole contribution is dropped. Taking // the first N of a sorted list was deterministic but it handed the choice of @@ -423,9 +446,9 @@ func (m *Manager) Status() []Status { c := m.conns[name] s := Status{Name: name, Tools: len(c.tools)} if c.cfg.Command != "" { - s.Transport, s.Target = "stdio", strings.Join(append([]string{c.cfg.Command}, c.cfg.Args...), " ") + s.Transport, s.Target = transportStdio, strings.Join(c.cfg.argv(), " ") } else { - s.Transport, s.Target = "http", c.cfg.URL + s.Transport, s.Target = transportHTTP, c.cfg.URL } if c.client != nil { s.Connected = true @@ -442,21 +465,10 @@ func (m *Manager) Status() []Status { // Call runs server's tool with args. Args come from the router and nothing // else; there is no path here through which a note or a fact could travel. func (m *Manager) Call(ctx context.Context, server, tool string, args map[string]any) (string, error) { - m.mu.Lock() - c := m.conns[server] - m.mu.Unlock() - if c == nil { + cl, cfg, _, configured, known := m.lookup(server, tool) + if !configured { return "", fmt.Errorf("%w: %s", ErrNoServer, server) } - m.mu.Lock() - cl, timeout, known := c.client, c.cfg.Timeout, false - for _, t := range c.tools { - if t.Name == tool { - known = true - break - } - } - m.mu.Unlock() if cl == nil { return "", fmt.Errorf("%w: %s", ErrNotConnected, server) } @@ -466,11 +478,30 @@ func (m *Manager) Call(ctx context.Context, server, tool string, args map[string if !known { return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } - cctx, cancel := context.WithTimeout(ctx, timeout) + cctx, cancel := context.WithTimeout(ctx, cfg.Timeout) defer cancel() return cl.CallTool(cctx, tool, args) } +// lookup reads one server's live state under the lock. Every call path needs +// the same four answers, and reading them in one critical section keeps a +// server that goes down mid-check from answering half yes. +func (m *Manager) lookup(server, tool string) (cl *Client, cfg ServerConfig, found Tool, configured, known bool) { + m.mu.Lock() + defer m.mu.Unlock() + c := m.conns[server] + if c == nil { + return nil, ServerConfig{}, Tool{}, false, false + } + for _, t := range c.tools { + if t.Name == tool { + found, known = t, true + break + } + } + return c.client, c.cfg, found, true, known +} + // Resources lists resources across connected servers. func (m *Manager) Resources(ctx context.Context) []Resource { m.mu.Lock() @@ -494,18 +525,11 @@ func (m *Manager) Resources(ctx context.Context) []Resource { // ReadResource reads one resource from one server. func (m *Manager) ReadResource(ctx context.Context, server, uri string) (string, error) { - m.mu.Lock() - c := m.conns[server] - var cl *Client - var timeout time.Duration - if c != nil { - cl, timeout = c.client, c.cfg.Timeout - } - m.mu.Unlock() + cl, cfg, _, _, _ := m.lookup(server, "") if cl == nil { return "", fmt.Errorf("%w: %s", ErrNoServer, server) } - cctx, cancel := context.WithTimeout(ctx, timeout) + cctx, cancel := context.WithTimeout(ctx, cfg.Timeout) defer cancel() return cl.ReadResource(cctx, uri) } @@ -559,32 +583,18 @@ var ErrNeedsArgs = errors.New("mcp: tool needs named arguments") // router picks tools by name similarity and the description a human reads is // server-written too. func (m *Manager) CallPositional(ctx context.Context, server, tool string, args []string) (string, error) { - m.mu.Lock() - c := m.conns[server] - var schema json.RawMessage - found, readOnly, bindable := false, false, false - configured, connected := c != nil, false - if c != nil { - connected = c.client != nil - for _, t := range c.tools { - if t.Name == tool { - schema, readOnly, found = t.InputSchema, t.ReadOnly, true - bindable = contains(c.cfg.AllowTools, tool) - break - } - } - } - m.mu.Unlock() - if !found { + cl, cfg, t, configured, known := m.lookup(server, tool) + if !known { if !configured { return "", fmt.Errorf("%w: %s", ErrNoServer, server) } - if !connected { + if cl == nil { return "", fmt.Errorf("%w: %s", ErrNotConnected, server) } return "", fmt.Errorf("%w: %s/%s", ErrToolGone, server, tool) } - named, err := bindPositional(schema, args, readOnly && bindable) + bindable := t.ReadOnly && contains(cfg.AllowTools, tool) + named, err := bindPositional(t.InputSchema, args, bindable) if err != nil { return "", err } @@ -620,23 +630,30 @@ func bindPositional(schema json.RawMessage, args []string, bind bool) (map[strin if !bind { return nil, fmt.Errorf("%w: %q, and a guessed argument goes only to a read-only tool named in allow_tools", ErrNeedsArgs, name) } - tail := strings.TrimSpace(strings.Join(args, " ")) - if tail == "" { - return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) - } - switch prop.Type { - case "string", "": - return map[string]any{name: tail}, nil - case "integer", "number": - n, err := strconv.ParseFloat(tail, 64) - if err != nil { - return nil, fmt.Errorf("%w: %q wants a number, got %q", ErrNeedsArgs, name, tail) - } - return map[string]any{name: n}, nil - default: - return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, prop.Type) - } + return bindOne(name, prop.Type, args) default: return nil, fmt.Errorf("%w: %s", ErrNeedsArgs, strings.Join(s.Required, ", ")) } } + +// bindOne puts the whole positional tail in the one required property. The +// tail is spoken words, so only a scalar can hold it and anything else is +// refused rather than coerced. +func bindOne(name, typ string, args []string) (map[string]any, error) { + tail := strings.TrimSpace(strings.Join(args, " ")) + if tail == "" { + return nil, fmt.Errorf("%w: %q", ErrNeedsArgs, name) + } + switch typ { + case "string", "": + return map[string]any{name: tail}, nil + case "integer", "number": + n, err := strconv.ParseFloat(tail, 64) + if err != nil { + return nil, fmt.Errorf("%w: %q wants a number, got %q", ErrNeedsArgs, name, tail) + } + return map[string]any{name: n}, nil + default: + return nil, fmt.Errorf("%w: %q is a %s", ErrNeedsArgs, name, typ) + } +}