Add config package for shared base configuration - #181
Conversation
Services in the ToolHive ecosystem each hand-roll their own YAML config loader. Give them a common BaseConfig (service name, log level) plus a strict generic Load() helper to embed and extend, so only truly universal fields live here and each service still owns its own schema and validation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Give BaseConfig a freeform Environment field so services don't each invent their own ad hoc field for this. Left unvalidated and without a fixed set of values on purpose: deployments name their environments differently, and a closed enum would force everyone into one org's naming. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
97403bb to
4c47d8d
Compare
Fixes GHSA-hrxh-6v49-42gf (High), which the grype-scan CI gate blocks on. Unrelated to the config package changes in this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rdimitrov
left a comment
There was a problem hiding this comment.
Review
What it does. Adds an Alpha config package with two pieces: an embeddable BaseConfig (ServiceName, LogLevel, freeform Environment) that consumers embed yaml:",inline" and extend, plus a generic Load[T any](path, *T) that strict-decodes YAML (KnownFields(true)). Also bumps grpc v1.82.0→v1.82.1 for a High-severity CVE that was blocking the grype-scan gate.
Overall: 👍 land it. The correctness surface is clean, the CVE bump unblocks CI, and Alpha gives room to iterate. Comments below are mostly about scope and API fit for the wider ecosystem.
Correctness
I verified the package's central claim by hand: strict decoding does reject unknown fields both at the top level (alongside the inline base fields) and nested, on yaml.v3 v3.0.1, so the guarantee holds. gopkg.in/yaml.v3 is already a direct dep — no go.mod churn needed. Nothing rose to a must-fix bug. Minor notes:
- Empty config file → cryptic error.
yaml.Decoder.Decodeon a zero-byte file returnsio.EOF, soLoadreturnsdecode config file X: EOFrather than a clean empty-then-validation-fails path. Low severity (an empty file failsValidate()anyway), but consider special-casingio.EOF. Validate()'sif c == nilguard is effectively dead in the documented inline-embed usage — you can't get a nil pointer to an embedded value field. Harmless, but it implies a calling pattern the package doesn't actually support.- Test gap (not a bug):
TestLoad_UnknownFieldRejectedonly exercises a nested unknown field. Since the headline feature is strict decoding of the inline top-level fields, a case with an unknown key at the document root would lock in the behavior I verified manually.
Does the idea make sense for the ecosystem?
Directionally yes — the duplication is real. toolhive already hand-rolls this exact pattern in pkg/vmcp/config/yaml_loader.go (yaml.NewDecoder(...) + KnownFields(true)), and Airlock's internal/config does the same. So the strict-YAML loader has ≥2 independent reinventions today; consolidating that is genuinely valuable.
But the value is lopsided, and a few things are worth deciding before this graduates past Alpha or gets treated as the ecosystem config layer:
-
Load[T]is the reusable piece;BaseConfigis thin. It's three fields, one (Environment) explicitly unvalidated with no helpers — closer to documentation than behavior. Its value only materializes if consumers converge on it, and they haven't yet:toolhive-registry-serveralready exposesserviceNamenested undertelemetry:, not at the top level, so adoptingBaseConfigthere means restructuring their schema. "Universal top-level fields" is more aspirational than observed. I'd resist growingBaseConfig's field set until ≥2 services embed it unchanged. -
Loadmay be too narrow for the biggest consumer. It's path-only (os.Openinside).toolhive's real loader reads bytes and then does substantial post-processing: env-var expansion (via the existingenv.Reader), auth-strategy resolution, composite-tool handling. Two cheap generalizations would widen adoption a lot:- Offer a
Decode(r io.Reader, *T)variant — embedded FS, remote config, in-memory tests, and byte-slice consumers all need this; path-only forces a temp file. - Decide where env-var interpolation (
${VAR}) lives, since every server-style consumer wants it and will otherwise keep hand-rolling aroundLoad.
- Offer a
-
The
string → slog.Levelparser is arguably in the wrong package.SlogLevel()bridges into the existingloggingpackage (which consumesslog.Leveler). Its natural home islogging, where the level type and defaults already live — otherwise config and logging can drift on what e.g."warn"means. -
Reuse breadth is currently speculative. One concrete near-term consumer (Airlock) plus one demonstrable duplication (toolhive) justifies the loader. It doesn't yet justify framing
BaseConfigas ecosystem-wide shared schema — a mild YAGNI flag. Alpha is the right hedge.
Suggested path before graduation: (a) add an io.Reader/Decode variant so byte-slice consumers like toolhive can adopt it; (b) move level-string parsing into logging; (c) prove BaseConfig by migrating a second service beyond Airlock (ideally converging registry-server's telemetry.serviceName) rather than expanding its fields speculatively.
Process nit: the grpc CVE bump is unrelated to the config package and bundled here only because it blocked the same CI gate — reasonable pragmatically, just flagging that the PR does two things.
🤖 Generated with Claude Code
Addresses PR #181 review: Load was path-only (os.Open inside), forcing byte-slice/embedded-FS/test callers through a temp file. Decode takes an io.Reader directly; Load is now a thin wrapper over it. Strict decoding stays the default, but AllowUnknownFields lets a future consumer opt out (e.g. a rolling deploy reading a newer config) without losing the default typo-catching behavior. Env-var interpolation is intentionally deferred — noted in doc.go rather than designed speculatively; better decided against the actual toolhive loader this package is meant to absorb.
New tests added "gw-1" occurrences past goconst's threshold.
Addresses PR #181 review: config shouldn't reference slog at all. BaseConfig.Validate now checks LogLevel against a plain string set with no logging-package dependency. The string->slog.Level parser moves to logging.ParseLevel, its natural home alongside the level type and defaults it already owns.
Summary
configpackage: an embeddableBaseConfig(service name, log level, freeform environment) plus a strict, generic YAMLLoad[T any]helper.BaseConfiginline (yaml:",inline") and add their own fields/validation on top — base fields don't grow to cover every service's schema.Environmentis a freeform string, deliberately unvalidated and without a fixed set of values — deployments name their environments differently, so a closed enum would force everyone into one org's naming.stacklok-enterprise-platform, which currently hand-rolls its own strict-YAML config loader (internal/config) and could consume this instead.google.golang.org/grpcv1.82.0 → v1.82.1, fixing a High-severity CVE (GHSA-hrxh-6v49-42gf) that was blocking thegrype-scanCI gate.Test plan
task lint— cleantask test(full repo) — all packages pass, including newconfigteststask license-check— SPDX headers presenttask grype-scan— clean after the grpc bumpBaseConfig, strict unknown-field rejection, missing-file handling, and freeformEnvironmentround-tripping through YAML🤖 Generated with Claude Code