Repository navigation
fix: failing CI checks left by upstream merges - #22
Merged
Merged
Conversation
Upstream's CIMD decorator resolves an omitted scope with registration.ValidateScopes so it matches DCR. The fork (16c2fba) changed ValidateScopes to grant the advertised scopes_supported set instead of failing when DefaultScopes do not fit, so CIMD now behaves like the fork's DCR. Update the test case and the stale comment to the fork behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Upstream's TestBuildServeConfigMapsSharedFields fails when a Config field is not set by buildServeConfig. The fork adds WriteTimeout and SessionOwnerAdvertiseURL to Config, but the fork's vMCP CLI builds the server with server.New and a full Config, never through Serve. List both fields as intentionally unmapped instead of adding fork fields to ServerConfig. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Upstream's Terminate marks a placeholder terminated with storage.Update, and its test stub injects failures on Update. The v0.29.3 merge kept the fork's stub, which only overrides Upsert and Create, so the placeholder fallback tests never saw a failure. Add the Update override from upstream; keep Upsert because fork session code still writes with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The restore tests from 7a6c9a2 were written for the fork's subject binding. Since the v0.29.3 merge, sessions bind to upstream's (iss, sub) identity tuple, read from the identity claims, so creating a session with only Subject set fails with "missing iss claim". Build the test identities with iss and sub claims; the restore code is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork (f22ee7f) makes the validating webhook middleware skip parsed MCP messages that are not requests, so client JSON-RPC responses pass through. Upstream tests added later (stacklok#5192 body size cap, HMAC secret resolution) build a ParsedMCPRequest without IsRequest, so the fork middleware skipped them and the assertions failed. Set IsRequest as the MCP parser does for real requests. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The fork (9b83a5e) names the operator RBAC objects after the Helm release so the prod and stg releases do not fight over one ClusterRole. Upstream's namespace-scope helm-unittest suite (stacklok#5475) asserts the toolhive-operator-manager-role name but renders with the default RELEASE-NAME release. Set the release name, as the default install suite already does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
govulncheck reports GO-2026-6603, GO-2026-6610, GO-2026-6611, GO-2026-6612 and GO-2026-6617 (HTTP/2 DoS and framing issues) in golang.org/x/net v0.58.0, all fixed in v0.60.0. The bump pulls newer golang.org/x/crypto, mod, sync, sys, term and text. go mod tidy also drops a duplicate go-openapi require line and stale go.sum entries left by the upstream merges. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The v0.28.3 merge kept redis.address as a deprecated alias of addr on MCPExternalAuthConfig (8d09330), so the CEL rule message reads "exactly one of addr (or its deprecated alias address) or sentinelConfig must be set". Upstream's RedisStorageConfig validation envtest expects the message without the alias and fails. Match the fork message until the alias is dropped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jgpelaez
approved these changes
Oct 12, 2026
jgpelaez
changed the title
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.
Summary
The PR checks on dp-stable merge PRs (#13, #19, #20, #21) fail for real reasons left behind by the upstream v0.28.3/v0.29.3 merges, so every future fork PR (including the upcoming v0.32.0 merge) starts red. This PR makes the checks green again. It is a dependency bump plus test fixes; the PR touches more than 10 files only because of
go.sum.ValidateScopesdefault an omitted scope toscopes_supported); comment incimd_decorator.goupdated.Config.WriteTimeoutandSessionOwnerAdvertiseURLas intentionally unmapped by upstreambuildServeConfig(fork CLI usesserver.New); documented inserve.go.Updateoverride to the Terminate storage stub; the merge kept the fork stub that only overrides Upsert/Create, so no failure was injected.issandsubclaims, since sessions bind on(iss, sub)since v0.29.3.IsRequest), because fork f22ee7f skips non-requests in the validating webhook middleware.release.name: toolhive-operatorin the operator namespace-scope helm-unittest, because fork 9b83a5e names RBAC per release (operator.rbacName).golang.org/x/netto v0.60.0 to clear govulncheck GO-2026-6603/6610/6611/6612/6617 (all in v0.58.0). This pulls newer x/crypto, mod, sync, sys, term, text;go mod tidyalso dropped a duplicate go-openapi require and stale go.sum lines (about 450 go.sum lines).addressalias).We stay on GitHub-hosted runners. The jobs that "did not run" on #20 were cancelled because no hosted runner was acquired (org capacity), not because of config, so there are no
runs-onchanges.Type of change
Test plan
task test)task test-e2e)task lint-fix)Manual testing and other checks run:
task helm-unittest: 50/50 passtask operator-test-integration: pass (after the Redis CEL commit)--repeat=4: 5/5GOTOOLCHAIN=go1.27.2: only GO-2026-5932, which is already ignoredtask license-check: passtext-embeddings-inference:cpu-latestimage on an arm64 laptop (CI preloads it), so they are unverified locally.Not run: helm
ct install.task lint-fixwith local golangci-lint v2.14.0 reports 38 pre-existing issues in untouched files (CI's pinned lint passed on #21).Does this introduce a user-facing change?
No
Special notes for reviewers
addressalias (decided in PR Add the v0.32.0 merge plan for the fork #21).ratelimit-testPending (8a6d20c) and the EmbeddingServer invalid PodTemplateSpec integration failure (b3b0da4). They only failed on the older PR ci: run PR checks on GitHub-hosted runners and fix tests inherited on dp-stable #13/fix(vmcp): restore bound sessions with the owner's live identity #19 heads.🤖 Generated with Claude Code