Repository navigation
Merge upstream v0.28.3 and v0.29.3 into dp-stable - #20
Merged
carlos-lopez-vecino merged 293 commits intoOct 6, 2026
Merged
carlos-lopez-vecino merged 293 commits into
carlos-lopez-vecino merged 293 commits into
Conversation
Update release files Release-Triggered-By: samuv
…tacklok#5080) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Add interactive TUI dashboard Add a terminal dashboard (thv tui) for managing MCP servers with: - Real-time workload list with status indicators and filtering - Tabbed panels for logs, server info, tools, proxy logs, and inspector - Tool inspector with form-based calling and JSON tree response viewer - Registry browser with search and detail view - Log search with case-insensitive highlighting and match navigation - Horizontal scrolling for long log lines - Keyboard-driven navigation with contextual help overlay The codebase is organized into focused files by area (inspector, registry, search, navigation) with shared form field helpers and sub-structs for clean state management. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add run from registry support in TUI Allow launching MCP servers directly from the registry browser. Press 'r' in the registry detail view to open a form with the workload name and environment variable fields pre-populated from registry metadata. Secret fields use password masking. On submit, the TUI spawns 'thv run' as a subprocess and refreshes the workload list on completion. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Cap tool description to 2 lines in inspector Truncate long tool descriptions in the inspector form to 2 lines with an ellipsis and [i] hint to view the full text in the detail modal. This prevents verbose MCP tool descriptions from pushing form fields off screen. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Sort environment variables in info panel Sort environment variable keys alphabetically before rendering in the server info panel. Go maps have non-deterministic iteration order, which caused env vars to shuffle on every 5-second refresh cycle. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add unit tests for TUI pure functions Cover core logic functions with 130+ subtests across 8 test files: registry (sanitize, filter), view helpers (wrap, truncate, count), inspector (required set, field values, result formatting), logs (split, diff), log formatting (parse, level style), JSON tree (parse, flatten, collapse, roundtrip), search (highlight), and form helpers (navigation, blur). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix security, memory, and usability issues in TUI Escape single quotes in buildCurlStr to prevent shell injection when the generated curl command is pasted into a terminal (CodeQL finding). Cap log buffers at 10k lines to prevent unbounded memory growth during long-running sessions. Bound horizontal scroll to the longest line. Extract shared newBackendClientAndTarget helper to deduplicate fetchTools/callTool setup. Remove unused parameter from renderRegistryOverlay. Complete the --help keybinding list (8 → 20 bindings). Document diffLines duplicate-line limitation and secrets process-visibility tradeoff. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Escape shell metacharacters in buildRunCmd clipboard output Single-quote and escape all dynamic values from registry metadata (server name, transport, permission profile, env var names) in the generated `thv run` command that is copied to clipboard. Without escaping, a malicious registry entry containing shell metacharacters could lead to arbitrary command execution when the user pastes the command into a terminal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Use suffix sequence matching in diffLines for duplicate log lines Replace single-line matching with a 3-line suffix sequence match to reliably detect the boundary between old and new log lines when the same message repeats. Falls back to single-line matching when prev has fewer than 3 lines or no sequence match is found. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address Copilot review comments in TUI - Fix --secret flag format: pass secret env vars as --env NAME=VALUE since the run form collects actual values, not secrets-manager references - Add height < 2 guard in View() to prevent index-out-of-bounds panic on very small terminal resize events - Remove unused base-frame parameter from renderHelpOverlay and update its comment to match actual behavior - Fix three UTF-8 backspace bugs (registry filter, sidebar filter, inspector tool filter) that sliced by byte instead of by rune - Rewrite highlightSubstring to operate on rune indices so Unicode case mappings that change byte length cannot desync slice positions - Add defer in tuiCmdFunc to emit OSC 111 background-colour reset on abnormal exit paths (panics) that bypass View()'s quitting branch - Add unit tests for buildRunCmd covering transport, permission-profile, required/optional env vars, and combined flag cases Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix golangci-lint failures in TUI code - gci: fix import block ordering in actions.go and json_tree.go - exhaustive: add all ContentType cases to formatInspResult switch; extract per-panel status bar hint helpers so renderStatusBar switch covers every activePanel value explicitly - gocyclo: suppress with nolint on inherently complex key-handler and renderer switch statements (handleInspectorKey, handleNormalKey, handleMsg, renderMain, renderInspResponse, renderStatusBar, renderJSONItem) - gosec G115: annotate uintptr→int conversions in help.go and spinner.go - gosec G204: annotate exec.CommandContext in actions.go - lll: break long runFromRegistry signature onto multiple lines - revive: add godoc comment to Model.View; rename unused width param to _ in renderTools - staticcheck: replace sb.WriteString(fmt.Sprintf(...)) with fmt.Fprintf - thelper: add t.Helper() to inline check closures in inspector_test.go - unparam: remove n parameter from makeFormFields (always 3) - unused: delete unreachable rebuildLogSearch and rebuildProxyLogSearch Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Generate CLI docs for thv tui command Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix bugs found in post-merge review - Guard runFormSubmit against empty fields slice before indexing fields[0] - Initialize toolsView viewport in New() to avoid zero-width renders before the first WindowSizeMsg arrives - Cap inspector slog buffer at 500 lines (same pattern as logLines) - Validate that env var and secret keys do not contain '=' before constructing --env NAME=VALUE arguments - Clarify buildRunCmd comment: --secret references a secrets-manager entry, not an inline value; distinguish from runFromRegistry --env path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix remaining golangci-lint failures - errcheck: wrap deferred fmt.Fprint to discard return values - gci/gofmt: fix struct field alignment in keys.go (Space, CopyNode); add missing final newline in model.go; add spaces around arithmetic operators in view.go lipgloss chain - gosec G115: annotate three IsTerminal uintptr→int casts in spinner.go - lll: shorten nolint comment in view_inspector.go to fit 130-char limit - staticcheck QF1012: replace remaining sb.WriteString(fmt.Sprintf(...)) with fmt.Fprintf in help.go (footerHint, RenderCommandUsage, renderParentHelp) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Replace vmcp BackendClient with mcp-go SDK in TUI The TUI talked to MCP servers through the vMCP aggregation client, which is designed for 1:N multi-backend routing. This was overkill for the TUI's 1:1 workload interaction: every operation created a new client, ran the full initialize handshake, loaded four auth strategies (all resolving to unauthenticated for localhost), and wrapped the transport in unused middleware layers. Switch to using the mark3labs/mcp-go SDK directly with a persistent client per selected workload. The client is created once on workload selection, reused for ListTools and CallTool, and closed when the user switches workloads or exits. This removes the vmcp dependency from pkg/tui/ entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Mark TUI command as experimental in help output Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Regenerate CLI docs after TUI experimental tag Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Coerce inspector field values to JSON Schema types inspFieldValues sent every form value as a string regardless of the tool's declared schema type, causing servers that validate input to reject calls with non-string parameters. Add parseFieldValue to convert strings to the correct Go type (int64, float64, bool, or parsed JSON for arrays/objects) before building the arguments map. Also validate required fields before the call and focus the first offending field on error so users get immediate feedback instead of a confusing server-side rejection. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove redundant inspector test cases Drop test cases that exercise the same code paths already covered by other cases: duplicate default-branch hits, same-branch boolean variants, stdlib behavior, and per-type coercion cases in TestInspFieldValues that duplicate TestParseFieldValue coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Move MCP client connect off the Update goroutine createMCPClient and connectMCPClient ran synchronously inside maybeLoadTools, which is called from Update. This blocked the entire TUI (input, ticks, log streaming) until the MCP server responded or timed out, violating Bubble Tea's non-blocking Update contract. Move the connect into a tea.Cmd that runs on a background goroutine and posts mcpClientReadyMsg on completion. The handler sets the client and kicks off the tools fetch, keeping Update responsive throughout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Guard runesTruncate against non-positive width A very small terminal width can drive the truncation limit to zero or below, causing a panic on the negative slice index r[:n-1]. Return the ellipsis immediately when n <= 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Cancel log and proxy-log contexts on quit The quit handler closed the MCP client but left log and proxy-log streaming goroutines running until process exit. Cancel both context functions so background goroutines stop promptly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove dead CLIHandler and document TUILogHandler limits CLIHandler was unused outside its own file. Remove it and its dependencies. Document that TUILogHandler is an end-of-pipeline handler whose WithAttrs/WithGroup methods intentionally return the receiver unchanged rather than accumulating state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Extract shared MCP client package The TUI and CLI each had their own MCP client creation logic with divergent behavior: the TUI hardcoded version "1.0.0" and skipped auto-detection, while the CLI had the full implementation. Extract the shared logic into pkg/mcp/client with a single Connect function that handles transport selection, auto-detection, and the initialize handshake using the real ToolHive version. Both the CLI and TUI now delegate to this package. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Replace runFromRegistry subprocess with in-process manager call runFromRegistry shelled out to thv run via os/exec, leaking secrets in /proc/cmdline and creating a drift surface between TUI and CLI behavior. The TUI already holds a workloads.Manager for stop, restart, and delete operations. Build a RunConfig from registry metadata using the same RunConfigBuilder pattern as the CLI and call the manager directly. Use context.WithoutCancel so launched workloads outlive the TUI session. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Replace filterRegistryItems with provider.SearchServers filterRegistryItems reimplemented substring matching that provider.SearchServers already provides, and missed tag matching. Delegate to SearchServers so the TUI and thv search share the same filtering semantics. The provider's in-memory cache makes per- keystroke calls safe without network overhead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Surface log polling errors and increase fetch window Silent error swallowing in StreamWorkloadLogs left the log pane frozen on workload removal or runtime restart. Emit errors as visible log lines with exponential backoff (1s to 5s) to prevent tight-looping. Increase logFetchLines from 50 to 500 to reduce line loss on chatty streams. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Tighten tautological test assertions TestLevelStyle, TestHighlightSubstring, and TestFormatInspResult only asserted NotEmpty or Contains(plainText), which would pass with broken implementations. Add ANSI escape verification, cross- level differentiation, and content-specific assertions. Add main_test.go to force ANSI256 color profile in test environments where lipgloss strips styling. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Stop search prompt from swallowing n and N keys handleSearchKey matched SearchNext/SearchPrev (bound to n/N) before the rune input case, so typing n or N during search query entry navigated matches instead of inserting the character. Remove those cases from the active-search handler; match navigation is already handled in handleNormalKey after the user commits with Enter. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove redundant required placeholder text The "(required)" suffix in the form field placeholder vanishes when the user starts typing. The label already renders a persistent red asterisk via renderFormField for required fields, making the placeholder suffix redundant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Add comment explaining best-effort curl preview args Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix lint errors after merging main Handle WorkloadStatusPolicyStopped in exhaustive switch statements, fix gci import grouping for toolhive-core imports, and reduce buildRunConfigFromRegistry cyclomatic complexity by extracting validateAndMergeEnvVars and permissionProfileName helpers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Fix gci import grouping for toolhive-core toolhive-core shares the github.com/stacklok/toolhive prefix and belongs in the same import group as toolhive packages, not with external dependencies. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Filter registry by short name when query starts with / Typing /github in the registry browser now matches only the short name after the last slash, so io.github.stacklok/github matches but io.github.stacklok/something-else does not. Queries without the / prefix still use SearchServers for full-text matching across name, description, and tags. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Remove trailing newlines flagged by linter Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Reset terminal background on SIGTERM/SIGINT The OSC 11 sequence that sets the TUI background persists after exit if the process is killed by a signal before the defer or View() can emit the reset. Install a signal handler that prints the OSC 111 reset before re-raising, covering terminal multiplexers and Ctrl+C during hangs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Handle clipboard errors and keep filter active after Enter clipboard.WriteAll errors were silently ignored while showing a success notification, misleading users on platforms without clipboard support. Check the error and show a failure notification when it occurs. filteredWorkloads only applied the filter while the prompt was active, so pressing Enter dismissed both the prompt and the filter. Filter whenever the query is non-empty so users can navigate the filtered list after confirming. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…stacklok#5081) Xcode (GitHub Copilot for Xcode) supports LLM gateway but not MCP client registration. Previously declared as a ClientApp constant, it was included in the generated swagger enum for the MCP API, misleading consumers into attempting unsupported operations. Introduce a separate LLMClientApp type for LLM-gateway-only tools and move the Xcode constant to it. The internal clientAppConfig entry uses an explicit ClientApp(Xcode) cast so the config table continues to work unchanged. Regenerate swagger docs to confirm xcode no longer appears in the enum. Closes stacklok#5073 Co-authored-by: taskbot <taskbot@users.noreply.github.com>
DetectedLLMGatewayClients() previously detected an installed AI tool by
checking whether its settings directory exists on disk. This produced
false positives when directories like ~/.claude or ~/.gemini were left
over from a prior uninstall or created by unrelated tooling.
Add an optional LLMBinaryName field to clientAppConfig. When set,
DetectedLLMGatewayClients() now requires both the settings directory
and the binary on $PATH (via exec.LookPath) before considering a tool
installed. Tools not on $PATH (e.g. Xcode, a macOS GUI app) leave the
field empty and fall back to the directory-only check.
Populate LLMBinaryName for claude-code ("claude"), gemini-cli ("gemini"),
cursor ("cursor"), vscode ("code"), and vscode-insider ("code").
Closes stacklok#5070
Co-authored-by: taskbot <taskbot@users.noreply.github.com>
* Add awsSts auth type support to vMCP The awsSts ExternalAuthType was defined in the CRD but had no implementation in the vMCP layer, causing DiscoverAndResolveAuth to fail with "unsupported auth type: awsSts" for any MCPServerEntry using AWS STS authentication. Add the full vMCP-side AWS STS auth path: - AwsStsConverter in the converter registry translates CRD configs to BackendAuthStrategy at session discovery time - AwsStsStrategy performs STS AssumeRoleWithWebIdentity + SigV4 signing with a per-config cache (map + sync.RWMutex, SHA-256 keyed) matching the TokenExchangeStrategy lifecycle - TokenProviderName field on AWSStsConfig enables upstream token selection when an embedded auth server is present on a vMCP (proxy runner and remote proxy are unaffected — upstream swap middleware already handles this); the vMCP controller auto-populates it with the first configured upstream provider when left empty - Export ExtractSessionName from pkg/auth/awssts to eliminate duplication between the middleware and strategy - Validator, yaml_loader, and factory registration all updated - Deep copy, CRD manifests, and API docs regenerated Closes stacklok#5018 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Address review feedback for AwsStsStrategy - Thread caller's context through getOrCreateContext instead of using context.Background, so cancellation propagates to NewExchanger - Add null byte delimiters between fields in buildAwsStsCacheKey to prevent field-separator ambiguity - Cache RequestSigner and sessionDuration in awsStsContext alongside roleMapper and exchanger; remove per-request signer allocation - Call ValidateConfig on cache miss in getOrCreateContext so Authenticate without prior Validate still gets a clear error - Add aws_sts test cases to TestInjectSubjectProviderIfNeeded covering auto-population, already-set not overridden, and nil AwsSts config Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Run `task docs` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * Address review feedback for AwsStsStrategy - Rename TokenProviderName to SubjectProviderName on AWSStsConfig and the runtime AwsStsConfig so it matches token_exchange's name for the same concept (which upstream provider's token to pull from Identity.UpstreamTokens). Aligns the CRD surface across strategies. - In the bearer-token fallback, use identity.Token instead of reading the outgoing request's Authorization header. The outgoing request is the one being constructed for the backend and is not guaranteed to carry the caller's header; identity.Token is the original incoming bearer captured by the auth middleware. Mirrors token_exchange. - Log a debug line when falling back to identity.Token. - Replace sort.Slice with slices.SortStableFunc in buildAwsStsCacheKey so mappings sharing a RoleArn but differing in Claim or Matcher keep their input order. Otherwise logically identical configs could hash to different keys and cause spurious cache misses. - Include cfg.GetSessionDuration() in the cache key. SessionDuration is baked into the cached awsStsContext, so omitting it would let two backends differing only in session duration share a cache entry and silently use the wrong value. - Add a success-path case to TestAwsStsStrategy_Validate. NewExchanger uses aws.AnonymousCredentials{} so it makes no network calls, and the previous test set had only error cases. - Change authtypes.RoleMapping.Priority from *int32 to *int so it actually mirrors awssts.RoleMapping.Priority (the role mapper uses math.MaxInt for nil-priority semantics in effectivePriority). Move the int32->int conversion to the CRD-to-runtime converter, where it belongs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Cover aws_sts in outgoing auth registry tests Add authtypes.StrategyTypeAwsSts to the strategy enumeration in both the registration check and the name-mapping table inside TestNewOutgoingAuthRegistry. The factory has registered five strategies since aws_sts was added, but the test only enumerated four — a regression that dropped aws_sts registration would have gone unnoticed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* thv llm: complete setup/teardown orchestration Add the missing pieces that turn the adapter scaffolding into a working end-to-end flow: - thv llm setup now accepts inline --gateway-url / --issuer / --client-id (and the other config flags) so "config set" and "setup" can be combined in a single command. Flags are merged into the persisted config before the rest of setup runs. - setup triggers an interactive OIDC browser login after configuring all detected tools, so the refresh token is stored before any tool or proxy tries to use it. - setup starts the localhost proxy as a background process when any proxy-mode tool (Cursor, VS Code, Xcode) was configured, and saves the PID to config so teardown can stop it. - teardown sends SIGINT to the stored proxy PID (if any) before clearing the PID from config. The PID is always zeroed in the final UpdateConfig call regardless of whether the signal succeeded. - pkg/llm.ProxyConfig gains a PID field (yaml/json: "pid") to hold the background proxy process ID across CLI invocations. E2E tests in cli_llm_setup_test.go cover: inline-flag config + tool patching, full teardown revert, single-tool targeted teardown, no-config error, and --purge-tokens token cleanup. Fixes stacklok#5031 * thv llm: complete setup/teardown orchestration - setup accepts inline --gateway-url / --issuer / --client-id (and the other config flags), merged into persisted config before running, so config-set and setup can be combined in a single invocation - OIDC browser login runs after tool config files are patched so the refresh token is cached before tools use it; proxy-mode tools receive a hint to run "thv llm proxy start" (foreground command, no daemon) - teardown reverts all (or a single named) tool config patches; --purge-tokens deletes cached OIDC tokens from the secrets provider - loginFunc injected into runLLMSetup for keyring-free unit tests - purgeCachedTokens and hasProxyMode extracted as helpers Fixes stacklok#5031 * fixes from review --------- Co-authored-by: taskbot <taskbot@users.noreply.github.com>
The v1beta1 MCPRegistrySpec exposes only configYAML, volumes, volumeMounts, pgpassSecretRef, displayName, and podTemplateSpec since stacklok#4705. REGISTRY.md was still documenting the typed sources[] / git / configMapRef / api / filter / syncPolicy fields directly under spec, which no longer exist on the CRD. - Rewrite Quick Start to use a minimal Kubernetes source via configYAML. - Add a Spec Reference table covering the actual CRD fields and a brief description of what lives inside configYAML. - Restructure Data Sources into Kubernetes / ConfigMap+file / Git / API subsections, each with a configYAML example. Move Git private-repo auth onto passwordFile + volumes/volumeMounts. - Add a PostgreSQL Authentication section covering pgpassSecretRef. - Update Filtering, Sync Operations, and Secret Management to reflect the configYAML path. - Replace the four legacy Examples blocks with a table linking to the canonical examples in examples/operator/mcp-registries/, and fix the See Also link to the renamed git-auth example. Signed-off-by: Radoslav Dimitrov <radoslav@stacklok.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…KEN PAT (stacklok#5093) Replace the long-lived RELEASE_TOKEN PAT with a GitHub App installation token (via actions/create-github-app-token) for both create-release-pr.yml and create-release-tag.yml. Update the toolhive-release skill reference docs to describe the new app-based setup. The installation token is short-lived (1h), repository-scoped, and avoids the GITHUB_TOKEN limitation where downstream workflows do not fire. Closes stacklok#4835 Signed-off-by: Nashon Steffen <nashon@stacklok.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
stacklok#5103) The MCPRemoteProxy spec accepts spec.resourceOverrides.proxyDeployment .imagePullSecrets via the ResourceOverrides type shared with MCPServer, but the MCPRemoteProxy controller never read it. Users configuring private-registry credentials silently got no effect: the proxy-runner Deployment and ServiceAccount were created with no image pull secrets and image pulls failed against authenticated registries. Mirror the MCPServer wiring: extract ImagePullSecrets from ResourceOverrides via a small helper and apply it to both the proxy-runner ServiceAccount (through rbac.EnsureRBACResourcesParams) and the Deployment pod spec. Add TestMCPRemoteProxyEnsureRBACResources_ImagePullSecrets covering both surfaces. Fixes stacklok#5099 Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
) * Extract shared OAuthTokenSource into pkg/auth/tokensource Both pkg/llm and pkg/registry/auth implemented the same three-tier OAuth token strategy (in-memory cache → secrets-cached access token → refresh token → browser OIDC/PKCE). This extracts that logic into a single shared package, pkg/auth/tokensource, and rewires both consumers as thin wrappers. As a side effect, the registry path now wraps its restored token source with PersistingTokenSource, matching the LLM path and ensuring rotated refresh tokens are re-persisted after a cache restore. Closes stacklok#5050 * fixes from review --------- Co-authored-by: taskbot <taskbot@users.noreply.github.com>
…cklok#5109) VS Code Insiders ships its CLI shim as `code-insiders`, not `code`. With `LLMBinaryName: "code"`, DetectedLLMGatewayClients() would fail to detect vscode-insider on machines that only have the Insiders build installed (no stable VS Code), causing `thv llm setup` to silently skip it. Adds TestRealClientConfigs_LLMBinaryNames as a regression guard that asserts the binary name for every LLM-gateway-capable tool in supportedClientIntegrations. Fixes stacklok#5089 Co-authored-by: taskbot <taskbot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…ok#5104) * Preserve user fields in EmbeddingServer podTemplateSpec merge The previous merge function copied only NodeSelector, Affinity, Tolerations, SecurityContext, ServiceAccountName, and the embedding container's SecurityContext. Anything else the user put on spec.podTemplateSpec.spec -- imagePullSecrets, additional volumes, priorityClassName, topologySpreadConstraints, runtimeClassName, init containers, sidecars -- was silently dropped. Replace the enumerated copy with a strategic merge patch on the raw user JSON (mirroring VirtualMCPServer's approach). The patch preserves every user-provided PodSpec field while controller defaults remain where the user did not set a value. Using the raw JSON avoids the nil-slice-becomes-empty-array problem that re-marshaling would hit. Add a regression test covering imagePullSecrets, priorityClassName, volumes, runtimeClassName, topologySpreadConstraints, and sidecar containers, asserting they all reach the resulting StatefulSet pod template. Fixes stacklok#5100 * Skip PodTemplateSpec merge on invalid user input Refusing to build the StatefulSet when the user-provided PodTemplateSpec is malformed left the EmbeddingServer stuck with no pod and broke the existing integration test that asserts a StatefulSet is still created in this case. The validation status condition (Phase=Failed, ConditionPodTemplateValid=False) is the user-facing signal for invalid input; the controller now treats merge failures as soft skips so the resource still produces an observable StatefulSet built from controller defaults. Restores the pre-existing tolerant behavior that the previous field-by-field merge had implicitly via NewPodTemplateSpecBuilder's silent error path. * Extract PodTemplateSpec strategic merge patch to controllerutil The marshal-then-StrategicMergePatch-then-unmarshal sequence was copy-pasted across the EmbeddingServer and VirtualMCPServer controllers. Pull it out into a single policy-neutral helper — `ctrlutil.ApplyPodTemplateSpecPatch` — and have both callers consume it. The helper deliberately does not pick an error policy. Each caller keeps its own: - VirtualMCPServer hard-fails: a patch error blocks Deployment creation, surfaced as a controller error. - EmbeddingServer soft-fails: a patch error is logged and the StatefulSet falls back to controller defaults; the user-facing signal is the existing PodTemplateValid=False status condition. Both behaviors are valid for their respective resources and changing either is out of scope for the EmbeddingServer fix in stacklok#5100. The helper's godoc documents the contract; each call site documents which side it picked and why. Drop the now-unused `*EmbeddingServer` parameter from `buildPodTemplate`. Its callers no longer reference it after the merge function was inlined into the patch step in the parent commit. Add unit tests for the helper covering the no-op, merge, and failure cases. Add two new integration-style tests on the EmbeddingServer controller: one for the soft-fail fallback (a payload that the builder accepts but StrategicMergePatch rejects) and one for the empty-object no-op (verifying controller defaults are preserved). * Test by-name merge into the embedding container Strategic merge patch merges containers by name. A user-supplied container called `embedding` is a separate code path from a sidecar with a different name: env and volumeMounts get merged *into* the controller's container rather than appended as a new entry. The existing sidecar test only covered the additive case. Add a regression case that supplies env and volumeMounts on a container named `embedding` and asserts both are present on the single resulting container, and that the controller-set image survives the by-name merge.
* Add explicit imagePullSecrets field to VirtualMCPServer VirtualMCPServer workloads previously had no first-class image pull secret field — users had to use the raw-JSON spec.podTemplateSpec escape hatch. That works but is undiscoverable in the CRD schema and inconsistent with MCPServer, which has spec.resourceOverrides.proxyDeployment.imagePullSecrets. This adds spec.imagePullSecrets directly to VirtualMCPServerSpec rather than introducing a nested ResourceOverrides struct, since this is the only override field today and adding scaffolding for future fields that may not arrive is speculative. The field is a []corev1.LocalObjectReference and propagates to: - the vMCP Deployment's PodSpec.ImagePullSecrets - the operator-managed ServiceAccount the vMCP server runs as When both spec.imagePullSecrets and spec.podTemplateSpec.spec.imagePullSecrets are set, the existing strategic merge patch handles overlap natively via the corev1.PodSpec.ImagePullSecrets patchStrategy:"merge"+patchMergeKey:"name". Distinct names are unioned; same names get the PodTemplateSpec entry (user override wins on overlap). The ServiceAccount is always populated from spec.imagePullSecrets — PodTemplateSpec has no notion of a ServiceAccount. The legacy podTemplateSpec workaround keeps working unchanged. Part of stacklok#5101 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Detect spec.imagePullSecrets drift on existing Deployment The drift-detection chain compared individual container fields, pod template metadata, and the user-provided podTemplateSpec hash, but never the deployed PodSpec.ImagePullSecrets. A user who edited spec.imagePullSecrets on an existing CR saw no rollout — the Deployment kept stale or missing image pull credentials. Direct equality on deployment.Spec.Template.Spec.ImagePullSecrets is not safe: the live list is the strategic-merge union of spec.imagePullSecrets and any imagePullSecrets the user added under spec.podTemplateSpec.spec.imagePullSecrets, so a direct compare would either flag spurious drift or miss real changes depending on PodTemplateSpec content. Mirror the existing podTemplateSpec hash pattern: store a SHA256 of vmcp.Spec.ImagePullSecrets as a deployment annotation and compare in deploymentNeedsUpdate. The hash normalizes by sorting on Name so a reorder is not treated as drift. PodTemplateSpec drift remains covered by podTemplateSpecNeedsUpdate, so together the two annotations cover every input that influences PodSpec.ImagePullSecrets. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document strategic-merge semantics on vmcp imagePullSecrets Strengthen the godoc on VirtualMCPServerSpec.ImagePullSecrets to spell out the strategic-merge-by-name behavior with PodTemplateSpec, the fact that the operator-managed ServiceAccount only sees this field (not the union with PodTemplateSpec), and the current divergence from MCPRegistry's atomic-replace strategy. Tracked for alignment in stacklok#5111. Regenerate CRD manifests and crd-api.md to pick up the longer description. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Add vmcp imagePullSecrets integration tests Cover three flows the unit tests can't reach because they only run the Deployment builder in isolation, not the full reconcile loop: - Create-time propagation: a VirtualMCPServer with spec.imagePullSecrets produces a Deployment whose PodSpec.ImagePullSecrets and a managed ServiceAccount whose ImagePullSecrets both reflect the configured set. - Update-time rollout (regression for the drift-detection fix): editing spec.imagePullSecrets on an existing CR converges the Deployment and SA to the new list. - Combined-source semantics: spec.imagePullSecrets and spec.podTemplateSpec.spec.imagePullSecrets together yield the strategic-merge union on the Deployment, while the SA reflects ONLY spec.imagePullSecrets. Run with envtest (the existing test-integration suite). No new bootstrapping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Add explicit imagePullSecrets field to MCPRegistry MCPRegistry workloads previously had no first-class image pull secret field — users had to pass them through the raw-JSON podTemplateSpec escape hatch. That works but is undiscoverable in the CRD schema and inconsistent with MCPServer, which has spec.resourceOverrides.proxyDeployment.imagePullSecrets. This adds spec.imagePullSecrets directly to MCPRegistrySpec rather than introducing a nested ResourceOverrides struct, since this is the only override field today and adding scaffolding for future fields that may not arrive is speculative. The field is a []corev1.LocalObjectReference and propagates to: - the registry-api Deployment's PodSpec.ImagePullSecrets - the operator-managed ServiceAccount the registry API runs as When both spec.imagePullSecrets and spec.podTemplateSpec.spec.imagePullSecrets are set, podTemplateSpec wins on the Deployment (atomic replace); the ServiceAccount always uses spec.imagePullSecrets, since podTemplateSpec has no notion of a ServiceAccount. This matches the existing podTemplateSpec merge semantics. The legacy podTemplateSpec workaround keeps working unchanged. Part of stacklok#5101 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Remove dead dedup loop in WithImagePullSecrets The map-based dedup logic in WithImagePullSecrets was a no-op: the option only ever runs against an empty defaultSpec inside the PodTemplateSpecBuilder, so the "existing" map was always empty. Replace it with a direct assignment now that the merge step in MergePodTemplateSpecs handles user-vs-default precedence atomically. Also rewrite the misleading function comment to match what the code actually does, and document the ImagePullSecrets atomic-replace rule on MergePodTemplateSpecs's godoc bullet list. * Warn about SA-vs-Deployment split brain on imagePullSecrets Strengthen the godoc on MCPRegistrySpec.ImagePullSecrets to call out that this field is the only path that reaches the operator-managed ServiceAccount. The legacy spec.podTemplateSpec.spec.imagePullSecrets populates the Deployment pod spec only and silently bypasses ServiceAccount-level credential injection used by GKE Workload Identity, OpenShift, EKS IRSA, and similar managed platforms. Regenerate CRD manifests and CRD reference docs so the warning lands in the user-facing documentation. * Cover spec.imagePullSecrets in MCPRegistry integration tests Existing integration coverage only exercised the legacy spec.podTemplateSpec.spec.imagePullSecrets path, leaving the new spec.imagePullSecrets field and the ServiceAccount path untested end-to-end. Add four scenarios that go through the controller and a real (envtest) apiserver: - spec.imagePullSecrets only -> Deployment pod spec carries the value - spec.imagePullSecrets only -> ServiceAccount carries the value - spec.imagePullSecrets updated -> both Deployment and SA rotate - spec.imagePullSecrets + podTemplateSpec.imagePullSecrets together -> Deployment uses the PodTemplateSpec override (atomic replacement), SA still tracks spec.imagePullSecrets This locks in the documented merge rule and proves the SA propagation that managed Kubernetes platforms (GKE Workload Identity, OpenShift, EKS IRSA) depend on. * Treat empty imagePullSecrets list as nil to preserve SA defaults When spec.imagePullSecrets is set to an explicit empty slice (kustomize or helm overlays often produce this unintentionally), the RBAC client was wiping the ServiceAccount's existing ImagePullSecrets. On OpenShift this destroys the auto-managed dockercfg entries and breaks private-image pulls in a way that's silent and undiagnosable from the field's docs. Normalize the contract in pkg/kubernetes/rbac so an empty Secrets or ImagePullSecrets slice is treated identically to nil — both leave the existing SA fields untouched. Document the equivalence on MCPRegistrySpec.ImagePullSecrets so users know recreating the resource is the supported way to clear SA-level pull secrets. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* Move llm setup/teardown orchestration to pkg/llm runLLMSetup and runLLMTeardown in cmd/thv/app/llm.go contained ~200 lines of business logic (login injection, rollback, atomic persistence, tool patching). Per cli-commands.md the CLI layer should be a thin wrapper. Move Setup, Teardown, LoginFunc, mergeToolConfigs, hasProxyMode, isTarget, and PurgeTokens to pkg/llm/setup.go. Introduce GatewayManager and ConfigUpdater interfaces so pkg/llm does not need to import pkg/client or pkg/config (which both transitively import pkg/llm). CLI adapters in cmd/thv/app/llm.go bridge the concrete types to these interfaces. Closes stacklok#5091 * fixes from review --------- Co-authored-by: taskbot <taskbot@users.noreply.github.com>
…tacklok#5114) Add missing encoding/json import PR stacklok#5107 added a json.Marshal call to virtualmcpserver_deployment.go without importing encoding/json, breaking the build on main: cmd/thv-operator/controllers/virtualmcpserver_deployment.go:733:20: undefined: json Add the missing import. No behavior change. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…prevent nil panic (stacklok#5098) Signed-off-by: wucm667 <stevenwucongmin@gmail.com>
The MCPServer and MCPRemoteProxy controllers correctly include spec.resourceOverrides.proxyDeployment.imagePullSecrets in newly built proxy Deployments, but their deploymentNeedsUpdate drift checks never compare Spec.Template.Spec.ImagePullSecrets. As a result, editing the field on a live CR did not roll the existing Deployment: the proxy- runner ServiceAccount was refreshed via EnsureRBACResources (so kubelet recovered for new pods through the SA path), but the pod-spec field on the live Deployment remained stale. In the user-managed- ServiceAccount branch, where RBAC reconciliation is skipped, the pod- spec field is the only mechanism — so that path stayed broken. Add a comparison in both controllers using equality.Semantic.DeepEqual so nil and empty slices are treated identically (matching Kubernetes' own serialization semantics) and reorder/add/remove on populated slices triggers a rollout. MCPRemoteProxy gets a new podSpecNeedsUpdate sub-helper alongside the existing containerNeedsUpdate / deploymentMetadataNeedsUpdate / podTemplateMetadataNeedsUpdate trio; MCPServer gets an inline check that mirrors the surrounding style of its single deploymentNeedsUpdate function. Each controller gets a table-driven unit test covering seven cases: the no-override baseline, the bug repro, the removal flow, the populated match, the nil-vs-empty asymmetry from both sides, and the slice-reorder case (which equality.Semantic.DeepEqual treats as drift). Two envtest integration tests per controller exercise the full reconcile-update path: create CR, assert Deployment, mutate imagePullSecrets, assert Deployment rolls. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…acklok#5105) * Add operator-level defaultImagePullSecrets plumbing Cluster operators frequently need a registry pull secret applied to every workload the operator spawns (proxy runners, registry API, vMCP servers, embedding servers). Today the chart only exposes imagePullSecrets for the operator's own pod, forcing users to set the secret on every CR or to mutate the namespace-default ServiceAccount. This change introduces a chart value, operator.defaultImagePullSecrets, that the operator picks up at startup via THV_DEFAULT_IMAGE_PULL_SECRETS and applies as a default to every workload it spawns. All five workload-spawning reconcilers consume the shared imagepullsecrets.Defaults value and merge it with the per-CR list at workload-construction time: MCPServer, MCPRemoteProxy, MCPRegistry (via registryapi.manager), VirtualMCPServer, and EmbeddingServer. Precedence rule: per-CR imagePullSecrets take priority on name collisions; chart-level entries are appended additively and deduped by Name. The CR-level slice is never mutated. EmbeddingServer places the chart defaults on the base PodSpec and lets strategic-merge-patch additively union the user's PodTemplateSpec entries (PodSpec.ImagePullSecrets is declared with patchStrategy:"merge",patchMergeKey:"name"). Drift detection on every controller routes through the same merge helper as the construction site so chart defaults do not flag perpetual reconcile loops. The Helm template renders operator.env before chart-managed env vars so a user-supplied entry cannot silently override a reserved name like THV_DEFAULT_IMAGE_PULL_SECRETS — Kubernetes keeps the last entry on a duplicate-named env. The startup parser logs a diagnostic when the env var is set but parses to nothing (typos like " , " or ",,,") so the misconfiguration is visible. Part of stacklok#5102 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address review: TOOLHIVE_ env var prefix and accept both shapes Rename THV_DEFAULT_IMAGE_PULL_SECRETS to TOOLHIVE_DEFAULT_IMAGE_PULL_SECRETS to match the operator container's TOOLHIVE_* convention before the env var ships and a rename costs a deprecation cycle. Accept both shapes in operator.defaultImagePullSecrets to mirror operator.imagePullSecrets above: - plain strings: ["regcred", "otherscred"] - object with `name`: [{name: regcred}, {name: otherscred}] Users who copy the [{name: foo}] pattern from operator.imagePullSecrets no longer get a silent THV_DEFAULT_IMAGE_PULL_SECRETS=map[name:foo] env var and ImagePullBackOff at runtime. Malformed entries (numbers, nested lists, objects without a `name` field) now fail the helm template render with a clear message instead of producing a bogus env var. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ok#5092) * Remove fabricated 1h expiry for non-expiring upstream tokens RFC 6749 §5.1 marks expires_in as RECOMMENDED, not REQUIRED. When a provider omits it, the client has no basis to invent an expiry. Slack user tokens and GitHub OAuth App tokens deliberately omit expires_in because they are genuinely non-expiring and have no refresh token. The fabricated 1h ExpiresAt caused guaranteed hourly re-authorization failures for those providers: the token appeared expired, a refresh was attempted with no refresh token, ErrNoRefreshToken was returned, and the token was dropped from the service. Remove defaultTokenExpiration and the fabrication block. Pass token.Expiry (the Go zero value when expires_in is absent) through directly so downstream code sees zero ExpiresAt and treats the token as non-expiring. Guard the expires_at log field to emit "none" instead of the misleading year-0001 timestamp. Fixes stacklok#5046 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix zero-ExpiresAt semantics in IsExpired/IsExpiredAt Both methods returned true for a zero ExpiresAt value because now.After(time.Time{}) is always true (year 1 is in the past). All production call sites guarded with !ExpiresAt.IsZero() before calling, so the bug was harmless, but the methods were latent traps for future callers. Embed the zero guard inside each method: zero ExpiresAt means the provider did not assert an expiry, so the token is treated as non-expiring. Add nil-receiver guards for consistency and defensive correctness. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix storage backends to not evict non-expiring tokens Both backends applied a default ~31-day TTL when ExpiresAt was zero, which would silently evict non-expiring tokens and prevent retrieval. memory.go: split StoreUpstreamTokens logic so zero ExpiresAt leaves the timedEntry expiry as the zero value (never evicted). Guard cleanupExpired to skip entries with a zero expiry. redis.go: initialise ttl to zero in marshalUpstreamTokensWithTTL and only set it for non-zero ExpiresAt. The Lua script already handles ttl=0 by calling SET without PX. Add PERSIST on the session index set when ttl=0 so a non-expiring token write removes any TTL a prior expiring-token write set on the index set, preventing the index from being evicted while the non-expiring token still exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add regression test for non-expiring upstream token handling Verifies that GetValidTokens returns a stored token with zero ExpiresAt unchanged and does not attempt a refresh. The failure signal is structural: no mockoidc user is queued, so any refresh attempt would cause the test to fail via a mockoidc error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Bound non-expiring upstream token storage to Fosite session lifetime Non-expiring upstream tokens (zero ExpiresAt — providers that omit expires_in) previously had no storage TTL, so they were never evicted by the cleanup loop. The only removal path was explicit deletion, which only happened on authorization error paths. Add SessionExpiresAt to UpstreamTokens. The callback handler sets it to the Fosite refresh token expiry; the refresher carries it forward on token rotation. Both MemoryStorage and RedisStorage use it as a fallback TTL when ExpiresAt is zero, ensuring non-expiring upstream tokens are evicted when the session would have expired. Tokens where both ExpiresAt and SessionExpiresAt are zero remain indefinitely stored — the existing behaviour for cases where the session expiry is unknown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Always set SessionExpiresAt to bound refreshed non-expiring tokens Previously SessionExpiresAt was only set in the callback when idpTokens.ExpiresAt was zero. This created a hole in the refresh path: if the initial callback got a token with non-zero ExpiresAt, the stored SessionExpiresAt remained zero. If a later refresh response omitted expires_in, the refresher carried the (zero) SessionExpiresAt forward, the new row had both ExpiresAt and SessionExpiresAt zero, and storage backends stored it without any TTL — leaking indefinitely. Make SessionExpiresAt unconditionally meaningful: set it on every callback as the Fosite session bound. Storage backends still only consult it as a fallback when ExpiresAt is zero, so behaviour for expiring tokens is unchanged. The refresh path is now safe even if a provider stops asserting expires_in on a later rotation. Add TestUpstreamTokenRefresher_PreservesSessionExpiresAt to lock down that the refresher carries SessionExpiresAt forward into the updated record — easy regression for future refactors. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix Lua TTL inversion that orphaned mixed-expiry session indexes The session-index set in Redis must outlive every per-provider key it points to. The previous logic conflated "fresh set just SADD'd" with "existing set previously PERSIST'd by a non-expiring member" because PTTL returns -1 in both cases. The condition `currentTTL < ttlMs` was therefore always true for -1, causing PEXPIRE to undo a prior PERSIST. Failure mode: store a non-expiring token (PERSIST'd index), then store an expiring token for the same session — the index re-acquired a TTL, evicted at the expiring token's expiry, and the non-expiring token's per-provider key (which has no TTL) became unreachable through GetAllUpstreamTokens and DeleteUpstreamTokens. This fired deterministically for any multi-upstream chain that had a non-expiring provider followed by an expiring one (e.g., GitHub PAT + Google). Distinguish the two cases by reading EXISTS BEFORE SADD. Pre-SADD existence is unambiguous: if the set didn't exist, our SADD just created it and we own the TTL decision; if it did exist, its current TTL summarises prior members and a -1 means "intentionally persistent, leave alone". Add tests for: - Reverse-order mixed-expiry (non-expiring → expiring) — the bug - Fresh expiring write applies index TTL — regression guard - Same-provider rewrite from non-expiring to expiring — documents the intentional limitation that the index stays PERSIST'd until DeleteUpstreamTokens cleans up Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Guard expires_at log fields against zero-time formatting Commit 824bc55's body promised "Guard the expires_at log field to emit 'none' instead of the misleading year-0001 timestamp" but only removed .Format(time.RFC3339) — slog still serialises a zero time.Time as "0001-01-01T00:00:00Z". Two log sites in oidc.go (during the authorization-code-exchange and refresh paths) were never touched and still called .Format() unconditionally. Add formatExpiresAt(t time.Time) to the upstream package: returns "none" for zero time, RFC 3339 otherwise. Apply at all four log sites (oauth2.go ×2, oidc.go ×2). Drop the now-unused time import in oidc.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add integration tests for non-expiring upstream token handling Two end-to-end regressions covering the cascade-prevention fix and the chain handler's per-leg storage write: * TestIntegration_FullFlow_NonExpiringUpstreamToken — drives the full /authorize → /callback → /token flow against a mock OIDC server whose token endpoint has been wrapped to strip expires_in from responses. Asserts the stored token has zero ExpiresAt, GetValidTokens succeeds without triggering a refresh, and post-call ExpiresAt is still zero. Catches regressions that re-introduce a default expiry anywhere in the HTTP-response → convertOAuth2Token → storage path. * TestIntegration_MultiUpstreamChain_MixedExpiryOrderings — table-driven, both orderings of mixed-expiry providers in a two-leg chain. Asserts GetAllValidTokens returns both providers regardless of order. Pins the chain handler's per-leg storage write and the convertOAuth2Token zero-Expiry path through the full HTTP flow. The Redis Lua TTL semantics remain covered by the unit-level tests in pkg/authserver/storage/redis_test.go. Helpers: * captureResponseWriter / stripExpiresInMiddleware — buffer the upstream token response and remove expires_in before forwarding. Lets us drive a non-expiring token response without forking mockoidc. * startMockOIDCNoExpiresIn — applies the strip middleware to a mock OIDC server. Deliberately does NOT queue a default user, so any unintended refresh attempt fails (the failure signal for the cascade-prevention test). * runChainFlow — extracted the two-leg chain navigation from TestIntegration_MultiUpstreamSequentialChain, including the state-preservation invariant. The original test now uses the helper, eliminating duplicated chain-navigation code. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Add miniredis-backed integration test harness Reusable infrastructure for driving the full /authorize → /callback → /token flow against Redis-shape storage, plus Redis-variant siblings for the two new integration tests added in this branch. The harness: * `withRedisBackedStorage()` functional option on `testServerOptions` * `testServer.Miniredis(t)` accessor that fails loudly if the option wasn't applied (avoids nil-deref foot-gun from a struct field that's only sometimes populated) * The three setup helpers (`setupTestServer`, `setupTestServerWithOIDCProvider`, `setupTestServerWithTwoUpstreams`) gained a variadic `...testServerOption` parameter; default behavior unchanged (memory storage) * Default Redis prefix `test:auth:{ns:test}:` matches production-shape cluster routing semantics (hash tag), even though miniredis ignores slots The Redis-variant tests: * `TestIntegration_FullFlow_NonExpiringUpstreamToken_Redis` — symmetry coverage of the convertOAuth2Token → Redis storage path. Drives the same flow as the memory variant against miniredis. * `TestIntegration_MultiUpstreamChain_MixedExpiryOrderings_Redis` — smoke test for the chain handler ↔ Redis storage path through real Lua execution in both orderings. Note on the Lua TTL inversion bug (commit fec89b0): the integration test intentionally does NOT cover that bug. It only fires when marshalUpstreamTokensWithTTL produces ttlMs == 0, which requires both ExpiresAt and SessionExpiresAt to be zero on the UpstreamTokens. Since callback.go unconditionally sets SessionExpiresAt = now + RefreshTokenLifespan (commit 1b3bc81), the integration flow always produces ttlMs > 0 and the buggy Lua branch is unreachable from a real auth chain. The Lua invariant is locked down at unit level by pkg/authserver/storage/redis_test.go, which can construct UpstreamTokens with both zero fields directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Re-anchor SessionExpiresAt on refresh for legacy zero rows The PR's invariant — SessionExpiresAt is set unconditionally on every callback so the refresh path can carry it forward as a storage TTL bound — holds for fresh writes after this PR ships. Pre-PR Redis rows decode SessionExpiresAt as zero because the field was absent from the persisted JSON. If such a legacy row is refreshed and the upstream rotation also drops expires_in, both bounds would be zero. The Redis backend would PERSIST the indexed row and the Memory backend would retain it indefinitely (Redis at least caps blast radius via the legacy 30-day per-key TTL; Memory has no backstop). Defensively re-anchor SessionExpiresAt to now+RefreshTokenLifespan when both bounds on the candidate write would be zero. RefreshTokenLifespan is threaded from the validated Config through the server struct into upstreamTokenRefresher — the same value the callback writer uses, so legacy rows transition into the post-PR invariant on first refresh rather than being persisted unbounded. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Document same-provider PERSIST rewrite trade-off The Lua script's index-TTL rule "leave a PERSIST'd set alone" is correct for the cross-provider mixed-expiry case but has a known limitation: when the SAME (sessionID, providerName) slot is rewritten from non-expiring to expiring, the index stays PERSIST'd even though the only persistence anchor is gone. Detecting that would require per-member TTL tracking, which we deliberately avoided; the index is reaped on DeleteUpstreamTokens or session GC anyway. The behaviour is pinned by the redis_test.go case "same provider rewrite from non-expiring to expiring keeps PERSIST'd until rewrite", but the production code never said why. A future maintainer tightening the rule would have hit that test failure and had to reverse-engineer the rationale from the test name. Spell out the trade-off next to the invariant so the connection is obvious. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Pin 1-second TTL clamp for deeply-stale upstream tokens Before the fix on this branch, marshalUpstreamTokensWithTTL fell through to DefaultRefreshTokenTTL (30 days) when time.Until(ExpiresAt) + DefaultRefreshTokenTTL was negative — meaning both the access token and its 30-day refresh grace had elapsed. The same silent over-retention applied to the SessionExpiresAt branch introduced in this series. The fix changed the floor to time.Second so deeply-stale rows evict promptly. Without a regression test, a future refactor could silently revert this clamp — cold-path callers (refresher races, admin rewrites, legacy-row migrations) would then observe a 30-day grace on tokens that should have been evicted, causing unintended token retention in Redis. Add two subtests to TestRedisStorage_UpstreamTokens, one for each branch, that store a token with stalePast = -(DefaultRefreshTokenTTL + 1h) and assert mr.TTL ≤ 2s. Both fail loudly if the clamp is reverted. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Pin wire-shape contract for legacy upstream token JSON Pre-PR Redis rows have no "session_expires_at" key. The read path correctly handles this via a != 0 sentinel, but nothing in the test suite exercised that path with raw legacy bytes. Without a pinning test, a future refactor — e.g. switching to json.NewDecoder().DisallowUnknownFields() or renaming the JSON tag — would silently break deserialization of existing rows. Add a subtest that hand-rolls a legacy JSON string (no "session_expires_at"), injects it into miniredis directly via mr.Set, calls GetUpstreamTokens, and asserts that all non-time fields round-trip correctly and that SessionExpiresAt.IsZero() is true. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix SessionExpiresAt godoc to mention all write sites The previous comment claimed only the callback handler writes this field, but the refresher also writes it: it carries the value forward unchanged on every refresh, and (after the legacy-row defensive re-anchor) sets it from RefreshTokenLifespan when both bounds are zero on the input. The new test "preserves SessionExpiresAt when provider omits expires_in" (refresher_test.go) and the legacy-row re-anchor case both depend on these behaviors. Documenting them on the field makes the contract discoverable without grepping for write sites. Addresses stacklok#5092 review F9. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* Allow OAuth2 upstreams to omit userInfo config Some OAuth 2.0 authorization servers — notably MCP authorization servers per the MCP authorization spec — expose no userinfo endpoint and issue opaque (non-JWT) access tokens, leaving no surface to resolve user identity from. With UserInfo previously required, the embedded auth server could not integrate any such upstream: token exchange would succeed and then the userinfo step would fail (e.g. 401 against an unrelated identity API), surfacing as ErrIdentityResolutionFailed and aborting the auth flow. Make OAuth2UpstreamConfig.UserInfo optional. When nil, BaseOAuth2Provider.ExchangeCodeForIdentity skips the userinfo call and synthesizes a deterministic, non-PII Subject from a SHA-256 prefix of the access token (35 chars, "tk-" prefix so synthesized subjects are recognizable in logs and JWTs). Name and Email remain empty in this mode. The synthesized subject is stable for the lifetime of the access token and rotates on re-authentication, which is sufficient for the proxy's session-key role. Scoped to OAuth2 only — OIDC providers always have an ID-token- derived subject and never reach this path. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Bypass UserResolver for synthetic identities Synthesized subjects (set by OAuth2 upstreams whose userInfo is nil) are derived from the access token and rotate on every re-authentication. The previous wiring fed them into UserResolver.ResolveUser, which falls through to createUserWithIdentity whenever (providerID, providerSubject) is unknown — i.e. on every re-auth, since the synthesized subject is never the same twice. For a long-running auth server fronting a synthesis-mode upstream, the `users` and `provider_identities` tables grow without bound and per-user state fragments across many internal IDs that all map back to the same human. Mark synthesized identities with a new Identity.Synthetic flag and have the callback handler use the synthesized Subject directly as an ephemeral session key when the flag is set, skipping both ResolveUser and UpdateLastAuthenticated. UpstreamSubject continues to flow into stored UpstreamTokens, so refresh-token rotation is unaffected (refresher.go already preserves expired.UpstreamSubject). Coverage: TestBaseOAuth2Provider_ExchangeCodeForIdentity asserts the synthesis path sets Synthetic=true; new TestCallbackHandler_SyntheticIdentity_BypassesUserResolver drives the full callback flow with a synthetic Identity and asserts neither the users map nor the provider_identities map gain a row, while upstream tokens still persist and the response still redirects with an authorization code. Addresses PR stacklok#5094 review comment. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Surface synthesis-mode upstreams in operator-visible state Address three PR review comments on the synthesis-mode fallback so a silent regression — operator drops or misnames the userInfo block by typo, stale CRD bundle, or copy-paste — is loud at every visible surface rather than only manifesting later as missing identity claims in proxyrunner audit trails. Per-comment changes: - WARN at provider construction (oauth2.go): NewOAuth2Provider emits a one-shot WARN when UserInfo is nil, calling out that synthesis mode is active and what it does (non-PII subject, no Name/Email). Logs once per provider construction, not per-request, so steady-state is quiet. - Status condition on the owning CRDs (mcpexternalauthconfig_types.go, controllers): new advisory ConditionTypeIdentitySynthesized published by both MCPExternalAuthConfigReconciler and VirtualMCPServerReconciler.validateAuthServerConfig, set True with reason OAuth2UpstreamWithoutUserInfo and the offending upstream names in the message when at least one OAuth2 upstream lacks userInfo, set False (AllUpstreamsHaveUserInfo) when every upstream has one, and removed entirely for non-embeddedAuthServer types or when AuthServer- Config is absent. Source-of-truth helper EmbeddedAuthServerConfig. SyntheticIdentityUpstreams returns the affected upstream names sorted for deterministic messages, so both controllers behave identically per the operator status-condition-parity rule. - Drop decorative userinfo tripwire (oauth2_test.go): the tripwire server's URL was never wired into the OAuth2Config under test, so the zero-hits assertion was vacuously true. Replaced with a comment explaining that the prefix-tagged Subject plus empty Name/Email assertions ARE the observable side-effect of the synthesis branch (the userinfo path populates Name/Email and would never produce a "tk-" subject), so the existing assertions cover the regression class. Coverage: - Unit test for EmbeddedAuthServerConfig.SyntheticIdentityUpstreams covering nil config, OIDC-only (skipped), OAuth2 with userInfo (skipped), single + multiple synthesizing upstreams (sorted), and mixed sets. - Reconciler tests for both MCPExternalAuthConfigReconciler and VirtualMCPServerReconciler.validateAuthServerConfig asserting the IdentitySynthesized condition's status, reason, and that synthesizing upstream names appear in the message — and that the condition is absent for non-embeddedAuthServer types. Addresses PR stacklok#5094 review comments #3156683849, #3156683855, and #3156683864. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Refine synthesis-mode contract docs and recognition Address four PR review comments tightening the externally observable contract of synthesis-mode subjects so downstream consumers (audit pipelines, status conditions, debug tooling) can reason about them without hardcoding implementation details, and so the docstrings accurately describe what the algorithm guarantees. - pkg/authserver/upstream/oauth2.go: export IsSynthesizedSubject(string) bool. Recognizing a synthesis-mode subject is part of the package's stable surface (it appears in JWTs, logs, and audit records); a predicate is the supported way for downstream code to detect it without reaching for the literal "tk-". The internal synthesizedSubjectPrefix constant stays unexported — the prefix value is an implementation detail behind the predicate. Doc comment for synthesizeSubjectFromAccessToken now narrows its security claim to what SHA-256 actually guarantees: the digest reveals nothing about the input beyond what an attacker who already has a candidate token could confirm by re-hashing, and the output is non-PII only assuming the upstream issues opaque (non-JWT) tokens. - pkg/authserver/upstream/types.go: rewrite Identity.Subject's doc to enumerate all three sources (OIDC ID-token sub, OAuth2 userinfo sub, OAuth2 synthesized "tk-" value) and add a stability-contract paragraph that distinguishes refresh-token rotation (Subject stable) from a fresh authorization code flow on a synthesis-mode upstream (Subject rotates). The previous one-liner ("the canonical user identifier from the upstream IDP") silently lied about the synthesis branch. - pkg/authserver/config.go: drop the cross-package symbol reference to BaseOAuth2Provider.ExchangeCodeForIdentity in OAuth2UpstreamRunConfig.UserInfo's doc — Go has no compile-time check that prose names exist, so a rename or move silently rots the comment. New wording describes the behavior (SHA-256 + "tk-" prefix, OAuth2-only, OIDC unaffected) directly. - pkg/authserver/upstream/oauth2_test.go: TestIsSynthesizedSubject table covers the round-trip case (any synthesizeSubjectFromAccessToken output is recognized) and false-positive guards (UUIDs, integer IDs, Atlassian-shaped account_ids, empty strings, "tk-" appearing mid-string). Strengthen the existing "empty token" subtest to assert the documented output shape (prefix + 32 hex chars) instead of only determinism — a regression that short-circuited empty input to "" would previously have passed. Addresses PR stacklok#5094 review comments #3156683871, #3156683884, #3156683893, #3156683900, and #3156683908. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Run `task docs` and `task crdref-gen` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Tighten synthesis-mode comments across the feature branch Pass over comment lines added in earlier commits on this branch to drop multi-paragraph explanations where one sentence carries the same meaning, remove duplication between Identity.Subject and Identity.Synthetic docs that drifted as both grew, condense test-function godoc that re-stated the assertions below them, and trim inline test comments where the t.Run name plus the assertion already conveyed intent. Specifically preserved: - Three-source enumeration and stability contract on Identity.Subject — reviewer feedback explicitly asked for them. - "Either-direction transitions count" in applyIdentitySynthesizedCondition's call site — non-obvious invariant that a future reader could undo. - "First reconcile adds the finalizer; second runs the body" in the controller test — explains the two-call dance. No behavior change. task lint-fix clean; pkg/authserver/... and cmd/thv-operator/... (excluding envtest integration) test suites pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Run `task operator-manifests` and `crdref-gen` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address synthesis-mode review feedback Three threads from PR stacklok#5094 review: - Refuse to synthesize a subject from an empty access token. Without this guard the well-known sha256("") constant could collapse distinct sessions onto a shared (UserID, ProviderID) storage bucket if a future code path ever bypassed the existing convertOAuth2Token check. Extract synthesizeIdentity() so the invariant has a single, testable enforcement point and replace the misleading "empty token still produces a stable subject" synthesizer subtest with one that asserts ErrIdentityResolutionFailed on the synthesis path. - Compute the IdentitySynthesized advisory above the validation guards in both reconcilers. The advisory is a pure function of the upstream provider field shape and was previously skipped on validation-failure paths, leaving a stale True with an upstream name the broken spec no longer mentioned. Hoist applyIdentitySynthesizedCondition above Validate() in MCPExternalAuthConfigReconciler and extract a parity helper applyAuthServerIdentitySynthesizedCondition that runs before validateAuthServerConfig in VirtualMCPServerReconciler. Transition tests pin the contract on both controllers. - Document synthesis-mode subjects in docs/arch/11-auth-server-storage.md so operators understand the tk-... UserID format, the UserResolver bypass, and the Redis KeyTypeUserUpstream reverse-index churn implication. task lint-fix clean; pkg/authserver/upstream/... and cmd/thv-operator/controllers/... test suites pass with -race. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update release files Release-Triggered-By: jhrozek Co-authored-by: toolhive-release-app[bot] <280093410+toolhive-release-app[bot]@users.noreply.github.com>
The Cedar authorizer hardcoded EntityTypeTHVGroup ("THVGroup") as the
entity type used to construct Client.Parents UIDs from JWT group/role
claims. Deployments whose policy vocabulary doesn't use "THVGroup"
had no way to switch without patching the authorizer.
Add an optional GroupEntityType field to ConfigOptions and thread it
through EntityFactory so deployments can use names like OrgRole or
Group that fit their policy vocabulary. Empty defaults to "THVGroup"
so existing deployments are unaffected.
Validate non-empty values up front in NewCedarAuthorizer, per the
constructor-validation rule in .claude/rules/go-style.md, so a typo
fails at startup with a message naming the offending input rather
than producing broken entity lookups at request time. Delegate the
identifier-grammar check to cedar-go's policy parser by round-tripping
a synthetic permit() against the configured type — Cedar's parser is
the source of truth for what a valid identifier looks like, and any
future grammar refinements upstream land here automatically. Reject
the "::" namespace separator ourselves: namespaced types are out of
scope until parseCedarEntityID handles them.
Warn at construction when entities_json contains entities of type
"THVGroup" while GroupEntityType is configured to a different value.
The mismatch causes Cedar's "in" operator to evaluate to false on
those stale entities — a silent deny that is hard to debug without
this diagnostic.
Closes stacklok#5072
* Add authserver DCR discovery and config surface Implements phase 2 (steps 2a/2b/2e) of the authserver Dynamic Client Registration story (stacklok#4978, sub-issue A of stacklok#5037). Purely additive: no behavior change, no CRD changes. - pkg/oauthproto: FetchAuthorizationServerMetadata with three-path RFC 8414 / OIDC Discovery fallback, bounded per-call timeout, RFC 8414 §3.3 issuer equality check, and ErrRegistrationEndpointMissing sentinel. Leaf-package invariant preserved (no pkg/networking import). - pkg/authserver: DCRUpstreamConfig struct plus DCRConfig field on OAuth2UpstreamRunConfig for operators who want runtime RFC 7591 registration instead of a pre-provisioned ClientID. - pkg/authserver: OAuth2UpstreamRunConfig.Validate and DCRUpstreamConfig.Validate enforce exactly-one-of for ClientID vs DCRConfig and DiscoveryURL vs RegistrationEndpoint; CommonOAuthConfig.Validate is unchanged. Address code review feedback on DCR discovery Fixed issues from code review of commit b408d6b: - HIGH: bare-path discovery test now uses a non-empty tenant path so all three well-known URLs are distinct, proving the third-iteration fallback actually runs. - HIGH: buildDiscoveryURLs deduplicates path-insertion and bare URLs when the issuer has no tenant path, avoiding a redundant round-trip and duplicate entries in the aggregated error. - MEDIUM: deferred body drain no longer wraps resp.Body in io.LimitReader, so large responses are fully drained for connection reuse; the JSON decode path still bounds accepted bytes. - MEDIUM: expanded FetchAuthorizationServerMetadata doc comment with a worked errors.Is example for the (metadata, ErrRegistrationEndpointMissing) return contract; tightened the corresponding test to assert the partial document fields. - MEDIUM: added TestFetchAuthorizationServerMetadata_InvalidIssuer covering empty issuer, malformed URL, missing scheme/host, http non-loopback, and ftp scheme; added TestFetchAuthorizationServerMetadata_AllowsLoopbackHTTP to lock the loopback carve-out. - MEDIUM: added TestFetchAuthorizationServerMetadata_TimeoutOverridesCallerContext that shortens the timeout via a new discoveryTimeout var and asserts the function honors its internal bound even when the caller passes context.Background. - MEDIUM: normalized struct-tag spacing on DCRUpstreamConfig to match the surrounding single-space style; kept //nolint:lll only on fields whose tag line actually exceeds 120 characters. - MEDIUM: replaced the 13-line Apache header with the 2-line SPDX header on pkg/oauthproto/discovery.go, discovery_test.go, and errors.go to match the project convention already used elsewhere in the package. Fix misordered nolint:paralleltest comment on timeout test The comment block above TestFetchAuthorizationServerMetadata_TimeoutOverridesCallerContext split a single sentence across the //nolint directive in the wrong order, so reading top to bottom the explanation appeared before the directive ended mid-phrase. Reword the prose onto three lines above the test and drop a single-line //nolint:paralleltest directive directly above func. - MEDIUM: readability fix only — no behavior, assertions, or public surface changed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Address PR stacklok#5041 review feedback Wire the new run-config validators into the load path and tighten the discovery fallback so feedback from review #4198420468 stops being a documentation-only promise. - Call OAuth2UpstreamRunConfig.Validate from buildPureOAuth2Config so the ClientID/DCRConfig mutual-exclusion rule actually runs at the RunConfig→upstream-config conversion boundary, not as dead code. - Move OAuth2UpstreamRunConfig.Validate and DCRUpstreamConfig.Validate next to Config.Validate to satisfy the public-methods-on-top rule. - Reject configs that set both InitialAccessTokenFile and InitialAccessTokenEnvVar; the docs already promised mutual exclusion and silent precedence would mask credential rotation. - Validate DiscoveryURL / RegistrationEndpoint via networking.ValidateEndpointURL so malformed or non-loopback-HTTP URLs fail at startup instead of at first DCR call. - Stop short-circuiting in FetchAuthorizationServerMetadata when an issuer-validated document is missing registration_endpoint: stash it and keep iterating so a later URL with a full document wins. Real IdPs sometimes advertise DCR only on the OIDC document. - Aggregate per-attempt discovery errors with errors.Join + %w so callers can still inspect causes through errors.Is / errors.As. - Use parsed.Path instead of parsed.EscapedPath when building discovery URLs so url.URL.String escapes the tenant exactly once; tenants with characters like spaces no longer become "%2520". Tests cover each change, including a regression test that pins the deduplication of path-insertion and bare RFC 8414 URLs for tenant-less issuers. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…cklok#5096) The upstream refresh path used golang.org/x/oauth2's TokenSource, which omits the `scope` parameter on refresh requests. RFC 6749 §6 says the AS SHOULD treat omission as equal to "the scope originally granted", but in practice some authorization servers — notably Microsoft Entra ID's v1 token issuer — silently narrow refreshed tokens to the user's default consent set (e.g. Microsoft Graph User.Read), dropping any custom resource scopes the original token carried. Real-world impact: an embedded auth server flow against Entra ID with a custom resource scope (e.g. `api://<app-id>/session:role-any` for Snowflake's external OAuth integration) issues fresh tokens with the correct scope, but every refreshed token comes back with `scp` reduced to `User.Read`. Downstream resources reject the refreshed token because the scope they enforce on is missing, even though the bearer token is otherwise valid. Users see ~1h of working access followed by silent failure until they re-authenticate. Send `scope` explicitly on refresh, populated from the configured scope set. Per RFC 6749 §6 this is permitted ("MUST NOT include any scope not originally granted by the resource owner") and is portable across compliant ASes. Replaces the TokenSource-based refresh with a direct POST to the token endpoint so the scope parameter can be set. Mirrors the parsing semantics of golang.org/x/oauth2 (caps `expires_in` at MaxInt32 to avoid time arithmetic overflow on non-spec emitters), preserves the existing refresh token when the AS doesn't issue a new one (RFC 6749 §6 allows this), and keeps the existing error-shape contract (`token request failed: <error>: <description>`). Tests: - new: refresh request includes the configured scope set verbatim - new: original refresh token is preserved when the AS omits one - existing tests pass without modification - integration test against mockoidc passes Signed-off-by: Dallin Stevens <stevens.dallin23@gmail.com>
…r bugs (stacklok#5116) * test(e2e): add all-client LLM gateway e2e matrix + fix secret provider bugs Fixes two bugs discovered while building the new test suite: 1. Add "secret" to informationalCommands so thv secret provider environment does not require a container runtime to be available. 2. Fix GetSystemSecretsProvider to call GetProviderType() before checking SetupCompleted, so the TOOLHIVE_SECRETS_PROVIDER env var is respected when interactive secrets setup has not been completed. Adds test/e2e/cli_llm_all_clients_test.go with 16 e2e tests (label: llm && clients) covering all 6 supported LLM gateway clients, proxy DNS-rebinding protection, port conflict handling, and 4 edge cases. Tests use an in-process mock OIDC server and fake browser stubs — no Docker or real credentials needed. Adds test/llm-mock-env/ for manual end-to-end testing with a local mock OIDC + LLM gateway. Closes stacklok#5115 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fixes from review * fixes from review --------- Co-authored-by: taskbot <taskbot@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* Wire Cedar admission seam into vMCP core Authorization runs only as HTTP middleware today (AuthzMiddleware + AnnotationEnrichmentMiddleware), so a tool can pass tools/list yet be refused on tools/call — the "list says yes / call says no" gap — because the two checks live on different paths. Re-platform that single Cedar decision into the core so List* (filter) and Call/Read/Get (deny) enforce it from one source. Implements issue stacklok#5438: - Add the Admission seam (interface + Cedar-backed adapter) in pkg/vmcp/core/admission.go. The core moved to its own package in stacklok#5437; the adapter wraps pkg/authz, which would cycle with the root vmcp package, so the seam lives here rather than pkg/vmcp/admission.go. - Build the authorizer from cfg.Authz via the same factory path the HTTP middleware uses (authorizers.GetFactory + CreateAuthorizer); no new policy language. A nil cfg.Authz yields an allow-all seam, matching the AuthzMiddleware != nil guard. - Re-inject the explicit identity and core-sourced Tool.Annotations into the ctx the authorizer reads, replacing AnnotationEnrichmentMiddleware on the domain path. - Keep optimizer meta-tools (find_tool/call_tool) exempt via cfg.PassThroughTools, mirroring the HTTP response filter. - Add cfg.ServerName/cfg.PassThroughTools to feed the seam. server.New is untouched: the live path still enforces via middleware in Phase 1, so double-enforcement only ever occurs in tests that exercise the core directly. stacklok#5441 removes the HTTP middleware. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address code review feedback Fixed issues from code review: - HIGH: newAdmission now rejects an empty ServerName when Authz is set, mirroring the live factory's fail-loud check (incoming.go:120) so a misconfigured composition root fails at New instead of silently changing Cedar resource-scoped policy semantics. - HIGH: corrected the AllowToolCall comment — the seam exempts the optimizer meta-tool name only; replicating the HTTP middleware's call_tool inner-tool authorization is documented as a stacklok#5441 call-path wiring requirement, not this seam's responsibility. - MEDIUM: sharpened the convertAnnotations comment to state the precise server->core import cycle that forces the (temporary, C2-style) duplication, and documented that EntitiesJSON normalization belongs to the authz.Config builder, not the generic factory path. - MEDIUM: added TestAdmission_AnnotationGatedDecisionMatchesListAndCall proving readOnlyHint-gated decisions match between ListTools and CallTool through the core; documented CallTool's no-hint fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Clarify admission seam parity comments Second-pass review follow-ups (all non-blocking MEDIUM): - Clarify that FilterTools' non-nil empty slice mirrors pkg/authz filterToolsByPolicy and that nil-vs-[] wire normalization is the Serve layer's concern — the allow-all seam returns input unchanged by design (byte-for-byte parity with the nil-authorizer no-op), not an oversight. - Name newCedarAuthzMiddleware as the sibling construction path that must stay in lockstep until stacklok#5441 collapses them into one shared constructor. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Extract config validation to keep New under complexity limit Rebasing onto main combined the admission-seam build in New with stacklok#5437's workflowsRequireElicitation check (added during that PR's review), pushing New's cyclomatic complexity to 16 (gocyclo limit 15). Move the input-validation guards into a validateConfig helper to bring New back under the limit; behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address admission seam PR review feedback Addresses stacklok#5459 review comments: - MEDIUM core_calls.go (3374390455): classify the authorizer-error branch of Call/Read/Get as ErrAuthorizationFailed (fail closed), mirroring the live authorizeAndServe; preserve the underlying error in the chain for diagnostics. - MEDIUM admission_test.go (3374390466): log through an injectable *slog.Logger (withLogger option) and capture via a buffer-backed logger instead of swapping the global slog default, removing the -race hazard vs parallel siblings. - MEDIUM admission_test.go (3374390470): capture call args in mockCall and assert they reach the authorizer; add a Cedar arg-gated end-to-end test through CallTool. - LOW admission.go (3374390475): add the nil-guard to the server convertAnnotations copy so the "keep in sync" claim is truthful (both retired in stacklok#5441). - LOW admission.go (3374390484): document the deliberate fail-closed divergence for a non-nil-but-empty-policies config vs the live allow-all. - LOW core_calls.go (3374390492): add a direct findAdvertisedTool table test and an end-to-end nil-branch CallTool test; correct the inaccurate "unroutable" comment. - LOW admission.go (3374390498, 3374390532): cite handleToolsCall and the EntitiesJSON default by symbol instead of drift-prone file:line. - LOW admission.go (3374390547): wrap CreateAuthorizer failures with ErrInvalidConfig for uniform classification with the sibling construction errors. - LOW core.go (3374390556): document ServerName as a required precondition when Authz is set (and ignored when nil). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Remove PassThroughTools optimizer carve-out from admission seam Addresses stacklok#5459 review (r3377688855, blocker): the PassThroughTools field carved an optimizer-specific hole into the generic admission seam but covered only list-time visibility — not call_tool inner-target authorization, find_tool response filtering, or inner-target annotation sourcing. Shipping that partial commitment would lock in "the seam knows about meta-tools" as the implicit answer without scoping the full optimizer-admission work. Make the seam optimizer-agnostic instead: drop PassThroughTools from Config, cedarAdmission, newAdmission, and delete TestCedarAdmission_PassThroughExempt. The full optimizer-admission integration is deferred to its own focused PR; until then stacklok#5441's composition root fails fast (ErrInvalidConfig) on (Authz + optimizer) so the combination never routes through this seam, and the optimizer keeps the HTTP middleware. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Add vMCP Serve skeleton and ServerConfig Introduce the transport-side entry point of the vMCP New/Serve split. Serve wraps an already-constructed core VMCP and returns the existing *Server, building the mcp-go server and applying the transport defaults that server.New applies today. The route mux and HTTP lifecycle are provided by the carried-forward Handler/Start/Stop, so observable behavior is unchanged and server.New is not yet routed through Serve. Implements changes for issue stacklok#5439: - Add ServerConfig (transport-only fields plus the cross-cutting TelemetryProvider/AuditConfig) and Serve in pkg/vmcp/server/serve.go - Validate nil cfg / nil core / nil SessionFactory via ErrInvalidConfig - Wire the core's idempotent Close into the server shutdown sequence - Guard the /status handler against a nil backend registry, which the transport does not own yet in this phase Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address Serve skeleton review feedback - Clarify the Serve doc comment: it builds only the mcp-go server and shutdown wiring; the route mux and HTTP lifecycle stay in the carried-forward Handler/Start until later phases relocate them - Document the Config fields buildServeConfig deliberately leaves unmapped (AuthzMiddleware, HealthMonitorConfig) and why - Add a reflection drift guard asserting every other Config field is populated by buildServeConfig, so a future field can't be dropped silently Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Tighten Serve skeleton per PR review Addresses stacklok#5467 review comments: - MEDIUM serve.go (3381738359): extract shared default consts (defaultServerName/Version/Host/EndpointPath) referenced by New and buildServeConfig, and assert against them in tests, killing the triplicated literals - MEDIUM serve.go (3381738361): drop the dead Config.StatusReporter mapping (set directly on Server like HealthMonitor) and add it to the drift guard's intentionallyUnmapped set - LOW-MEDIUM serve.go (3381738364): document the returned *Server's nil-field contract (must not be Started/served on "/" until later phases wire session/discovery/registry) - MEDIUM serve_test.go (3381738372): probe /.well-known/ for the clean JSON 404; soften the overstated "auth-chain absent" comment - MEDIUM serve_test.go (3381738374): note the drift guard checks presence only; cover SessionTTL/StatusReportingInterval value correctness in TestServePreservesExplicitConfig - LOW status.go (3381738382): slog.Debug in the nil-registry branch so an unexpectedly-nil registry on the New path is observable Also from the review body: add a positive /metrics-registered test and a combined nil-cfg+nil-vmcp validation case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The operator chart had no helm-unittest coverage, so a template change could silently alter a default-install resource and still pass `ct lint` and `ct install` (helm install succeeds on subtly wrong output). The operator-crds chart already has a suite from stacklok#5468; the operator chart did not. Add a default-install suite that renders the chart with no value overrides and asserts the expected output for every resource it creates (Deployment, ServiceAccount, ClusterRole, ClusterRoleBinding, and the leader-election Role/RoleBinding), plus that the HPA is absent. Image fields use matchRegex on the repo path so version bumps don't break the suite. Wire the operator chart into the `helm-unittest` task and ignore tests/ when packaging the chart. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Deprecate MCPRegistry CRD with runtime warning The MCPRegistry CRD is superseded by the toolhive-registry-server Helm chart, which installs the registry server directly. Signal the deprecation so users migrate before it is removed, while keeping the CRD fully functional. - Mark the live v1beta1 MCPRegistry with deprecatedversion:warning so kubectl warns on every interaction; repoint the stale v1alpha1 warning (previously "use v1beta1") at the Helm chart. - Emit a Warning event plus log line from the reconciler, once per spec change, steering users to the chart. Wire an EventRecorder into the reconciler, mirroring the other operator controllers. - Express the deprecation as prose, not the Go "Deprecated:" convention, so staticcheck SA1019 does not flag the operator's own internal uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Regenerate MCPRegistry CRD manifests and docs Pick up the MCPRegistry deprecation markers and doc comments via operator-manifests and crdref-gen. Adds deprecated/deprecationWarning to the v1beta1 CRD version and refreshes the generated API reference. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The default-install baseline locks in the happy path, but the operator chart's real risks are value-driven couplings that span templates: a regression in any of them passes `ct lint`/`ct install` silently. Add four scenario suites, each setting one install configuration and asserting the resulting behaviour: - autoscaling: HPA renders and the Deployment drops its static replica count; CPU/memory metrics are gated on their targets - namespace scope: WATCH_NAMESPACE carries the joined allowlist and one RoleBinding renders per namespace (zero on an empty allowlist) - defaultImagePullSecrets: string/object forms join, and invalid entries fail the render (a path ct lint/install cannot reach) - feature flags: experimental and storage-version-migrator env vars flip to their quoted-boolean values Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The POST /api/v1/workloads path calls BuildTelemetryConfigFromAppConfig
with no header flags, so headers configured through an
OTEL_EXPORTER_OTLP_HEADERS entry in the config's EnvVars never reached
cfg.Headers — the single field the OTLP exporter sends via WithHeaders.
That entry was only copied into the workload's EnvironmentVariables, so
every configured header (auth, tenant routing, env tags) was silently
dropped on the API path, and an auth-gated collector rejected every
export with 401.
Parse OTEL_EXPORTER_OTLP_HEADERS out of EnvVars into the structured
Headers map, splitting on the first '=' only so values containing '='
(base64 padding) or spaces ("Basic <token>") survive intact. Explicit
--otel-headers flags still take precedence.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Add failing custom-naming SA consistency test
Overriding operator.serviceAccount.name must keep the SA name consistent
across the created ServiceAccount, the Deployment's serviceAccountName,
and the role binding subjects. Today serviceaccount.yaml names the SA
from fullname and clusterrole/rolebinding.yaml hardcodes the subject, so
two of the four assertions fail. This commit captures the bug as a red
test; the template fix follows.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Route SA references through serviceAccountName helper
serviceaccount.yaml named the ServiceAccount from fullname, while
clusterrole/rolebinding.yaml hardcoded the subject and
leader-election-role.yaml used the raw serviceAccount.name. Overriding
operator.serviceAccount.name made these diverge: verified in a kind
cluster, the operator Deployment got zero pods ("error looking up
service account my-sa: serviceaccount not found").
Route all four references through the existing
toolhive-operator.serviceAccountName helper (use $ inside the rolebinding
range). This is a no-op on default values, where every reference already
resolves to toolhive-operator. Extend the custom-naming suite with the
empty-name fallback cases so the generated-name path is locked in too.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lok#5477) Extend operator chart coverage to the template logic most prone to silent regressions and tied to past incidents: - security context: hardened pod/container defaults (runAsNonRoot, readOnlyRootFilesystem, dropped caps, seccomp) must not regress - selector labels: the immutable Deployment selector is pinned so a helper change can't break `helm upgrade` on existing releases - optional metadata: pod/ServiceAccount annotations and labels inject correctly when set and are absent otherwise (cf. annotation-indent stacklok#4602) - probes: liveness /healthz and readiness /readyz on the health port - naming: fullnameOverride/nameOverride precedence for resource names vs name labels, and the release-name fallback - env precedence: user env renders and chart-managed reserved vars still win on a name collision Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5317) * Update module github.com/stacklok/toolhive-core to v0.0.23 * Regenerate swagger docs for toolhive-core v0.0.23 The toolhive-core v0.0.22 bump adds a Stateless field to the registry server types. Regenerate the API docs so the committed swagger matches the generated output and the Verify Swagger Documentation check passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Pin go-containerregistry to v0.21.5 toolhive-core v0.0.23 transitively pulls in go-containerregistry v0.21.6, which has a semaphore leak that never releases acquired slots, hanging image pulls of 5+ layers and freezing the E2E Tests Core (proxy) job. The fix is merged upstream (google/go-containerregistry#2308) but not yet released, so pin to the last known-good version until a fixed release ships. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Radoslav Dimitrov <radoslav@stacklok.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Reynier Ortiz Vega <reynier@stacklok.com>
* Renew public client TTL on read in Redis store Public clients registered via DCR get DefaultPublicClientTTL stamped on their Redis registration at registration time, and GetClient only reads it back. The TTL is therefore a fixed window from registration: an actively-used public client (e.g. a long-lived MCP desktop/IDE client that caches its client_id) is evicted DefaultPublicClientTTL after it registered, regardless of use. Its next token request then fails at the token endpoint with invalid_client, and the only recovery is to fully re-register the client. Renew the TTL to DefaultPublicClientTTL on a successful GetClient for public clients, turning expiry into a sliding window of inactivity. Abandoned clients still expire after the window, preserving the anti-bloat intent of RegisterClient; confidential clients have no TTL and are left untouched. Renewal is best-effort: a failure is logged and does not fail the lookup. The in-memory backend stores clients without a TTL, so it is unaffected. Add a unit test covering both the public-client renewal and the confidential-client no-TTL invariant. Signed-off-by: Mani Japra <mani@muonspace.com> * Renew client TTL on token success, not on read Review flagged that renewing the public-client TTL inside GetClient fires on the unauthenticated front-channel GET /oauth/authorize read (fosite's NewAuthorizeRequest), not only on the token endpoint. Public clients have no secret, so any caller who knows a public client_id could keep its registration alive indefinitely by polling authorize, defeating the anti-bloat TTL the renewal was meant to preserve. Move renewal to the proven-use signal: a successful token exchange or refresh. Add RenewClientTTL to the ClientRegistry interface — Redis EXPIREs the public client's key (confidential clients and missing keys are left untouched), the in-memory backend is a documented no-op — and call it best-effort from TokenHandler after NewAccessResponse succeeds. GetClient returns to a pure read. Regenerate storage mocks for the new method. Update tests to exercise RenewClientTTL directly and assert that a successful token exchange renews the public client's TTL. Signed-off-by: Mani Japra <mani@muonspace.com> * Fix struct comment alignment to satisfy linter --------- Signed-off-by: Mani Japra <mani@muonspace.com> Co-authored-by: Chris Burns <29541485+ChrisJBurns@users.noreply.github.com>
…5480) Bump toolhive-core from v0.0.23 to v0.0.24. The new version's module requirements pull forward several transitive dependencies (aws-sdk-go-v2, go-chi, OpenTelemetry SDK, zap, and others) via go mod tidy. Also ignore docs/superpowers/ planning artifacts. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getFilePath constructed file paths with filepath.Join but never verified the result stayed within basePath. A name like "../../../etc/passwd" would resolve outside the state directory, making GetReader, GetWriter, CreateExclusive, Delete, and Exists all vulnerable to path traversal. Fix: after filepath.Join (which calls filepath.Clean to resolve ".." components), check that the result starts with basePath+separator using the same containment pattern already established in pkg/fileutils/contained.go. Update the #nosec G304 comments to reflect that the check is now actually enforced. Closes stacklok#4736
) * Wire SDK hooks and session creation into Serve Step P2.2 of the vMCP New/Serve split: the three mcp-go SDK hooks and the two-phase session-creation wiring are SDK-lifecycle concerns that belong in the transport layer, not the core VMCP domain object. This relocates them into Serve so they execute identically when Serve is exercised directly. server.New keeps its own copy until Phase 3, so its signature and observable behavior are unchanged. Implements changes for issue stacklok#5440: - Register OnRegisterSession + OnBeforeListTools/OnBeforeCallTool hooks in Serve, delegating to the unchanged *Server receiver methods - Build the transport session manager, session data storage (memory/Redis, THV_SESSION_REDIS_PASSWORD) with a close-on-error leak guard, and the vMCP session manager inside Serve - Add ServerConfig.SessionManagerConfig (pre-built *sessionmanager.FactoryConfig) and the shared BackendRegistry; fold the now-redundant SessionFactory/ OptimizerFactory/OptimizerConfig slots into FactoryConfig - Add Serve-level tests driving the relocated hooks + two-phase creation via the SDK lifecycle, plus buildSessionDataStorage coverage; the HTTP suite stays on server.New as its parity gate Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Address code review feedback Fixed issues from code review: - HIGH: Split the t.Setenv redis case into its own non-parallel test and made TestBuildSessionDataStorage parallel, resolving the tparallel lint failure - MEDIUM: Validate BackendRegistry in Serve (fail loudly with ErrInvalidConfig instead of nil-panicking inside the OnRegisterSession hook); add a nil-registry validation case and a testMinimalServeConfig helper - MEDIUM: Document on SessionManagerConfig that the composition root must validate workflow definitions before assembling FactoryConfig, since Serve no longer runs validateWorkflows Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Fix Serve session-manager leak and tighten Serve tests Addresses stacklok#5471 review comments: - LOW serve.go (3383233591): build the transport sessionManager after the fallible buildSessionDataStorage/sessionmanager.New calls so its cleanup goroutine cannot leak on a Serve error path (symmetry with the storage guard) - MEDIUM serve_session_test.go (3383233579): add a Serve-level test covering the cross-pod empty-store inject branch of lazyInjectSessionTools (and the pod-local no-op branch), the path the prior tests did not reach - LOW serve_session_test.go (3383233598): remove dead mu/lastSession test state - LOW serve_session_test.go (3383233608): rename the storage-error test to match what it asserts (error returned when session manager construction fails) - LOW serve_session_test.go (3383233616): bind the redis test assertion to the connection-failure wrap so an unrelated error can't satisfy it Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Wire RFC 9728 discovery into HTTP and stdio transports
All three transport backends (SSE, streamable HTTP, stdio) now accept
an optional authInfoHandler for OAuth protected resource metadata
(RFC 9728) and a prefixHandlers map for routes that must be reachable
outside the auth middleware chain.
The /.well-known/ path is always registered so OAuth clients receive a
clean JSON 404 when auth is off rather than Go's default plain-text 404.
The factory forwards both settings from Config to the StdioTransport it
creates.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Address review feedback on transport auth discovery tests
- Replace context.Background() with t.Context() at all Start(ctx) call
sites in the four new proxy test files (t.Cleanup closures keep
context.Background() since t.Context() is already cancelled there)
- Consolidate six flat factory-Create tests into one table-driven test
- Add TestNew{HTTPSSEProxy,HTTPProxy}_ExactWellKnownBeatsSubtree to pin
the ServeMux specificity invariant: an exact /.well-known/* prefix
handler from the embedded AS must win over the /.well-known/ subtree
PRM mount, and oauth-protected-resource must still reach the PRM handler
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
…lok#5210) Address Redis Cluster mode review follow-ups Three small follow-ups from the PR stacklok#5153 review that were deferred so the core cluster-mode support could land: - Document the cluster slot invariant for storeUpstreamTokensScript. The script reads oldUserID from KEYS[1] inside its body to keep the user-set bookkeeping atomic, so the user-set keys are constructed dynamically from ARGV[4] rather than declared as KEYS. The new comment makes the {ns:name} hash-tag requirement explicit, so a future refactor that rebuilds the prefix without the tag will be caught in review rather than silently regressing on standalone Redis and exploding with CROSSSLOT on a real cluster. - Filter SMEMBERS results in GetAllUpstreamTokens, DeleteUpstreamTokens, and GetLatestUpstreamTokensForUser to entries that share the storage instance's keyPrefix, warn-logging anything dropped. A stray un-prefixed member (legacy data, an external admin op, a test fixture) would today surface as CROSSSLOT under cluster while passing on standalone; this turns it into a logged warning. The defensive filter is a pure helper with table-driven tests, plus a behavior test that captures slog output to prove the wiring at all three call sites. - Align the operator CRD `tls` doc with the storage-layer comment so that crd-api.md and `kubectl explain` both describe TLS as applying to "Redis/Valkey master or cluster nodes" — previously cluster-mode users reading the CRD field would not realise this same field configured TLS for cluster-node connections. Regenerated CRD YAML, Helm templates, and crd-api.md to match. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
She has departed the company, so remove her from all code owner rules. Every affected path retains at least one remaining owner. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update release files Release-Triggered-By: reyortiz3 Co-authored-by: toolhive-release-app[bot] <280093410+toolhive-release-app[bot]@users.noreply.github.com> Co-authored-by: reyortiz3 <reynier@stacklok.com>
…tacklok#5485) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: reyortiz3 <reynier@stacklok.com>
Add release-notes automation with Slack announce Cutting a ToolHive release is followed by two manual chores: writing polished release notes and announcing the release in Slack. Both are repeatable and were being done by hand each time. This adds a workflow that fires on the `release: published` event (the same event that already drives releaser.yml) and: - Runs the existing `release-notes` Claude skill to analyze every merged PR in the range, cross-reference issues, and dispatch expert subagents for breaking-change assessment, producing copy-pasteable markdown. - Delivers it review-then-publish: the public release notes are not overwritten. The markdown is uploaded as an artifact, written to the job summary, and posted as a comment on the release PR for a maintainer to review and apply. - Posts a single Slack announcement reusing the theme summary the skill already emits, plus a link to the full release notes, via the same incoming-webhook pattern as the existing failure notification. Also supports workflow_dispatch with a tag input for dry-running against an already-published release. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Surface unauthenticated proxy default in logs and docs When an MCPServer or MCPRemoteProxy is deployed without OIDCConfigRef (or any other auth source), the proxy falls back to LocalUserMiddleware and forwards every request under a synthetic local-user identity with no credential check. This unauthenticated fallback is intentional, but it was silent: the only signal was a debug log, and the README claimed unconditional "identity enforcement per request" (GHSA-hfrv-94x5-85p2). Make the state visible without changing the default behavior: - Raise the no-auth fallback log in GetAuthenticationMiddleware from Debug to Warn, naming the consequence and how to enable auth. - Document the unauthenticated default on the OIDCConfigRef field of both MCPServer and MCPRemoteProxy; regenerate the CRD API reference. - Reword the README so identity enforcement is stated as conditional on configuring an authentication source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Correct crdref-gen working directory in operator rule The rule said to run `task crdref-gen` from `cmd/thv-operator/`, but the task resolves its config path relative to the repo root and fails from that directory. Document the correct invocation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Regenerate operator CRD manifests Propagate the OIDCConfigRef security note into the generated CRD YAML descriptions for MCPServer and MCPRemoteProxy. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update release files Release-Triggered-By: reyortiz3 Co-authored-by: toolhive-release-app[bot] <280093410+toolhive-release-app[bot]@users.noreply.github.com>
Brings in the hub restart fixes (stacklok#5064 sorted StatefulSet env, stacklok#5300 proxyrunner startup probe) and the session-timeout fixes (stacklok#5117). This merge is the fallback build for PAIINFRA-205: restart fixes without v0.29's session binding change. PAIINFRA-217. Fork behaviour carried through the conflicts: - Redis `address` -> `addr` (v0.27): `address` stays as a deprecated alias on MCPExternalAuthConfig. CEL accepts either (not both). The auth-server RunConfig still reads the old `address` key, so an image can start from a RunConfig written by the previous operator. Both aliases go in M3 (PAIINFRA-228) after PAIINFRA-219 moves the values. - Dragonfly: upstream re-added a Lua script for upstream-token writes. The fork's WATCH/MULTI path (907d704) is kept, with upstream's stacklok#5092 index-TTL rules for non-expiring tokens ported into it. - Redis clients now come from toolhive-core. The session store passes the ACL username from sessionStorage.usernameRef. The operator injected THV_SESSION_REDIS_PASSWORD twice (upstream stacklok#5286 plus the fork). One builder now injects username and password after the resourceOverrides env, and the drift check uses the same order. - Session TTL: RunConfig session_ttl (upstream) wins, then the fork's TOOLHIVE_PROXY_SESSION_TTL, then the default. For vMCP the --session-ttl flag wins over the config-file sessionTTL. - Operator chart pdb.yaml uses the renamed toolhive-operator.* helpers (stacklok#5245). Dropped: 4cca582 (covered by upstream stacklok#5335) and 690846d (upstream has primaryUpstreamProvider since v0.27.1; the inline location is still read). FORK.md records both. CRDs, RBAC, CRD API docs, swagger (swag rc5, as pinned) and chart READMEs are regenerated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in upstream's (iss, sub) session identity binding (stacklok#5306, stacklok#5378) and the v0.29 fixes. On top of the v0.28.3 merge, this is the build for PAIINFRA-205. PAIINFRA-217, absorbs PAIINFRA-223. Fork behaviour carried through the conflicts: - Session binding: upstream's binding replaces the fork's sub-based HMAC binding (e492cbf, dropped). VMCP_SESSION_HMAC_SECRET is now ignored. Sessions stored with the old token-hash key are invalidated on read, so clients re-initialize once after the upgrade. - Restore keeps the fork's live-identity rule (7a6c9a2): when the restoring caller owns the stored (iss, sub) binding, the session is rebuilt with that caller's identity; otherwise the context identity is cleared. The session-owner URL survives restore. - The fork's "bound session" checks (refresh and backend reconcile) read the identity binding through sessiontypes.HasBoundIdentity. - Terminate keeps the fork's local-session branch and takes upstream's remote full-session delete and placeholder Update path. - Upstream moved vMCP telemetry into internal/backendtelemetry; the fork's session-cardinality gauges move to the server package. - Upstream's new server.Serve (not used by the fork's vMCP binary) registers the fork's session hooks; upstream tests for lazyInjectSessionTools and CacheCapacity are dropped with it. - DCR keeps the server's supported scopes as the default when the client omits scope; the image manager keeps the digest-pull backport on upstream's single moby client. The Redis `address` alias from the v0.28.3 merge stays until M3 (PAIINFRA-228). CRDs are additive against v0.29.3. CRDs, RBAC, CRD API docs, swagger and chart READMEs are regenerated. FORK.md is updated. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
davidrubio-dp
approved these changes
Oct 6, 2026
carlos-lopez-vecino
deleted the
PAIINFRA-217-merge-upstream-toolhive-v-0-28-3-and-v-0-29-3-into-dp-stable
branch
October 6, 2026 06:44
This was referenced Oct 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Merge with "Create a merge commit". Do not squash or rebase. The PR holds two merge commits, and the first one (
0b8ddd18b, v0.28.3) is the fallback build.Summary
The hub keeps restarting, and sessions break when an OAuth token is refreshed. Upstream fixed both after v0.23, and the fork doesn't have the fixes yet. This PR brings them in for PAIINFRA-217 (epic PAIINFRA-205; it absorbs PAIINFRA-223). Nothing is deployed by this PR. The rollout follows in PAIINFRA-218, 220 and 221.
0b8ddd18bMerge upstream v0.28.3. This adds the restart fixes (fix(#5063): deterministically order env vars in DeployWorkload stacklok/toolhive#5064 sorted StatefulSet env, fix(operator): add startup probe to proxyrunner deployment stacklok/toolhive#5300 proxyrunner startup probe) and the session-timeout fixes (Add --session-ttl flag and fix three session timeout bugs stacklok/toolhive#5117). It is the fallback build: the restart fixes without the session-binding change.03863441Merge upstream v0.29.3. This adds upstream's(iss, sub)session identity binding (vMCP terminates MCP session on every legitimate access-token refresh stacklok/toolhive#5306, Bind vMCP sessions to OIDC identity, not raw token bytes stacklok/toolhive#5378) and the v0.29 fixes.Fork behaviour carried through the conflicts.
FORK.mdand the two commit messages have the details.address→addr(v0.27):addressstays as a deprecated alias onMCPExternalAuthConfig. CEL accepts either one, but not both. The auth-server RunConfig still reads the oldaddresskey. Both aliases go in M3 (PAIINFRA-228), after PAIINFRA-219 moves the values toaddr.907d7040d) stays, with upstream's Fix non-expiring upstream token handling and storage TTL bugs stacklok/toolhive#5092 index-TTL rules ported into it.sessionStorage.usernameRefis passed to the toolhive-core client.THV_SESSION_REDIS_PASSWORDwas injected twice, once by upstream fix(operator): inject THV_SESSION_REDIS_PASSWORD for MCPServer stacklok/toolhive#5286 and once by the fork. Now one builder injects the username and password afterresourceOverrides, and the drift check uses the same order.session_ttlwins, thenTOOLHIVE_PROXY_SESSION_TTL, then the default. For vMCP, the--session-ttlflag wins over the config-filesessionTTL.e492cbfad. Three fork behaviours stay on top of it:pdb.yamluses the renamedtoolhive-operator.*helpers (Namespace operator.* Helm helpers to prevent umbrella chart collisions stacklok/toolhive#5245).4cca58224: covered by upstream Preserve fresh per-request identity in vMCP backend transports stacklok/toolhive#5335.690846dd8: upstream hasprimaryUpstreamProvider. I checked toolhive-config:cognitois only set whereauthServerConfigdeclares acognitoupstream.e492cbfad: replaced by the upstream binding.server.Serve: the fork's vMCP binary doesn't use it. It compiles and registers the fork's session hooks. Two upstream tests for features the fork replaced (lazyInjectSessionTools,CacheCapacity) are dropped.Type of change
Test plan
task test): not complete. Tests that open loopback listeners (miniredis, httptest) could not run in the local sandbox. Every other local failure was environment-only (no container runtime, no keychain). CI must be green before merge.task test-e2e)task buildcommands, run offline) andgo vet ./...pass.helm templateoutput for the operator chart includes the PDB when it is enabled.sha-03863441...sha-0b8ddd18b9f9API Compatibility
v1beta1API. I reviewed the CRD diff against v0.29.3 by hand: it is additive or relaxed only (theaddressalias, rules that accept either field, and the existing fork fields).mcpwebhookconfigsis included. I didn't runcrd-schema-checkerlocally; the API Compatibility CI job runs it.Does this introduce a user-facing change?
Yes, from v0.29.3:
VMCP_SESSION_HMAC_SECRETis ignored.MCPExternalAuthConfigacceptsredis.addr.redis.addressstill works but is deprecated.Special notes for reviewers
These areas need the most scrutiny:
pkg/vmcp/session/factory.go: theRestoreSessionlive-identity rule on top of the binding.pkg/vmcp/server/sessionmanager/session_manager.go:Terminate.pkg/authserver/storage/redis.go:queueUpstreamIndexTTL.cmd/thv-operator/controllers/mcpserver_controller.go: the order of the credential env vars.The diff is over the 400-line guideline because this is an upstream merge, which is an allowed exception.
🤖 Generated with Claude Code