stacklok / stacklok/toolhive

vmcp: accept injected *slog.Logger via Config (avoid sharing slog.Default with the embedder)

Open
#5,381 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

enhancement go vmcp
Dominant language
Go
Stars
2.2k
Forks
300
Avg merge
1d 15h
Merged PRs (30d)
184

Description

Problem

pkg/vmcp/server library code uses package-level slog calls (slog.Info, slog.Warn, slog.Error, etc.) throughout. These read slog.Default() at call time. There is no public field on vmcpserver.Config for an embedder to inject a *slog.Logger.

For library consumers that embed pkg/vmcp/server inside a multi-component Go process with its own logger context, this means vmcp's logs share the embedder's process-wide slog.Default(). Useful when that's what the embedder wants; friction when the embedder wants per-component tagging, separate sinks, or different log levels per component.

The workarounds today:

  • Mutate slog.SetDefault() globally before vmcp lifecycle calls, then restore after. Fragile (any future internal vmcp callsite that reads the default at a different point breaks the save/restore window), and clobbers the embedder's own logger config for the duration.
  • Accept that vmcp logs land in the embedder's default sink with no per-component metadata.

The standalone cmd/vmcp binary doesn't run into this because it owns its process and sets slog.SetDefault() once at startup.

Requested fix

Add Logger *slog.Logger to vmcpserver.Config. Behavior:

  • If Config.Logger == nil: current behavior preserved. vmcp's package-level slog calls fall back to slog.Default().
  • If Config.Logger != nil: vmcp uses the injected logger for all internal logging. Internally this likely means routing the package-level helpers through a private function that prefers Config.Logger when set.

This is backward-compatible (additive Config field) and gives library consumers a clean opt-in injection point.

Suggested API

type Config struct {
    // ... existing fields ...

    // Logger, when non-nil, is used for all vmcp internal logging.
    // When nil, vmcp's package-level slog calls fall back to
    // slog.Default() (current behavior).
    Logger *slog.Logger
}

The zap-side equivalent can land as a second additive field (ZapLogger *zap.Logger) or, preferably, vmcp uses an internal slog→zap adapter so a single Config.Logger covers both. Either is fine from a consumer perspective.

Acceptance

  • Existing tests pass; standalone cmd/vmcp binary behavior unchanged when Config.Logger is left nil.
  • A new test confirms that, with Config.Logger set to a fresh *slog.Logger, vmcp's emissions go to the injected handler rather than the package default.
  • A note in pkg/vmcp/doc.go (or the server README) documents the library-consumer pattern.

Why this matters

The change is backward-compatible and unblocks a clean library-embedding story without forcing every consumer to wrap vmcp lifecycle calls in save/restore boilerplate or to accept vmcp's logs sharing the embedder's default sink unmodified.

Happy to send a PR if the API shape above is roughly the direction you'd accept; flag any deviations before I start.


Edited 2026-05-26: the original framing claimed vmcp itself mutates slog.SetDefault / zap.ReplaceGlobals at startup. On closer inspection that mutation is consumer-side (in a library-embedding consumer's own bootstrap), not in pkg/vmcp/*. The underlying request stands: an injectable Config.Logger is the clean fix.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg/vmcp/server by locating Config, the package-level slog calls, and any zap-side logging paths; trace how Config reaches lifecycle code. Run the existing server tests before adding coverage for an injected logger and the nil fallback. Done means injected emissions use the configured handler, existing behavior and cmd/vmcp remain unchanged, and pkg/vmcp/doc.go or the server README documents the pattern.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend
Issue type
Feature
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
56/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.