feat(ci): tag-blocking release QA gate — server-type matrix + invariants (Spec 081 T1)#819
Open
Dumbris wants to merge 11 commits into
Open
feat(ci): tag-blocking release QA gate — server-type matrix + invariants (Spec 081 T1)#819Dumbris wants to merge 11 commits into
Dumbris wants to merge 11 commits into
Conversation
…ipt (Spec 081 T1) Adds cmd/mcpfixture, a single fixture binary for the release QA gate's server-type matrix (FR-006/FR-007): serves the deterministic echo and ping tools over stdio, streamable-http, and legacy SSE, wired through mark3labs/mcp-go's server package so each transport matches the exact client contract mcpproxy pins (internal/upstream/core). ping returns a per-process monotonic counter plus a random per-process instance_id so kill/restart cells (FR-007d) can prove reconnection hit a fresh instance. SIGTERM/SIGINT exit cleanly on every transport; SIGPIPE is ignored so a parent closing our pipes cannot turn shutdown into a broken-pipe death. Adds scripts/gate/build-fixture-image.sh + cmd/mcpfixture/Dockerfile: CGO_ENABLED=0 static linux build, FROM scratch image tagged mcpfixture:gate — zero third-party pulls (FR-009 infra preflight fails loudly when Docker is absent). The image intentionally has no ENTRYPOINT: mcpproxy's isolation layer appends the container command explicitly (docker run ... docker.io/library/mcpfixture:gate /mcpfixture --transport stdio), verified against internal/upstream/core/isolation.go BuildDockerArgs / TransformCommandForContainer and documented in the script. Tests (FR-022, -short friendly, exec the real binary): per-transport handshake + tools/list + echo/ping round-trip + SIGTERM-then-restart with counter reset and instance_id change; stdio SIGTERM clean-exit. Decisions made under the zero-interruption policy: preferred mcp-go's server package over hand-rolled framing; SSE endpoint event stays relative (mcp-go client resolves it against its base URL); oauth cell reuses tests/oauthserver as-is (not rebuilt here).
…e tools The MCP spec default for an un-annotated tool is destructiveHint=true, which makes mcpproxy's Spec-018 intent validation reject the tool through the call_tool_read variant. The Spec 081 release-gate matrix and its OAuth cell both drive fixtures via call_tool_read, so annotate the deterministic fixture tools (mcpfixture echo/ping and tests/oauthserver echo/get_time) as read-only, non-destructive, closed-world. Related to Spec 081 (release-qa-gate) T1.
…reate
AddServerRequest has always declared (and documented) an Isolation field, but
only the PATCH/update path mapped it into the ServerConfig — handleAddServer
silently dropped it. A caller therefore could not opt a host-run stdio server
OUT of isolation on create when global docker_isolation.enabled=true: the
server was force-wrapped in a container with its host command path and failed
to start ("stat <host-path>: no such file or directory"). Surfaced by the
Spec 081 release-gate quarantine invariant, which adds a fresh stdio fixture
via POST while the docker matrix cell has global isolation enabled.
Map req.Isolation on create exactly like the update path (toConfig), so the
field means the same thing on both verbs. Add regression coverage for opt-out,
image override, and the omitted (do-not-touch) case.
Related to Spec 081 (release-qa-gate) T1.
Add the release-qualification gate driver (cmd/release-gate) and the report
schema/merger (internal/gatereport) that assemble the Spec 081 US1/US2 checks
into one machine-readable verdict.
Driver subcommands:
- matrix: boots the candidate core against five local fixtures
(stdio/http/sse/docker/oauth) and runs FR-007 (a)ready
(b)tools listed + discoverable via retrieve_tools (c)tool
call round-trips through /mcp AND POST /api/v1/tools/call with
a caller X-Request-Id (d)kill+restart+re-call. Docker cell
fails infrastructure-classified when 'docker info' fails
(FR-009, never skip/fall back); oauth cell exercises an
initial authorization and a forced token refresh against the
short-TTL mock IdP (FR-008); each cell retries <=2 and reports
flaky on pass-on-retry (FR-010). Leaves the core running behind
a --state-file for the invariants stage.
- invariants: attaches to that live core and asserts FR-011 (100% of issued
calls resolve in the activity log by request id), FR-012
(token/usage/telemetry counters strictly increase under
traffic), FR-013 (add mid-run -> quarantined -> call blocked ->
approve -> round-trips), then FR-014 upgrade-in-place from the
latest STABLE release (downloads the host tarball, runs the OLD
binary on a scratch dir, SIGTERM+WAIT on the BBolt lock, starts
the candidate on the same dir, asserts servers/quarantine/
index survive).
- run-suite: wraps an existing suite entry point (FR-003) as a fragment.
- report: merges fragments against a hardcoded manifest into
gate-report.json + a markdown summary; a missing blocking
fragment is a FAIL (FR-004, no silent skips), reserved T2-T4
slots record not-run, flaky counts green, unexpected fragments
fail-closed.
Empirically confirmed against a live core that a caller-supplied X-Request-Id
is NOT persisted on tool_call activity records, so the activity invariant
issues the correlated call via POST /api/v1/tools/call in addition to the
MCP-native call and correlates by argument nonce + core-recorded request id,
recording the limitation in the report rather than hiding it.
Gate cores run with DO_NOT_TRACK=1 so the real downloaded release binary in
the upgrade check never emits production telemetry on local/dry-run
invocations (CI=true only covers GitHub runners); the in-process
builtin_tool_calls counters FR-012 reads stay live.
Negative-case coverage (FR-022): activity invariant fails on a missing/
unresolvable request id; flat counters fail; a pre-approved server fails the
quarantine invariant; the upgrade preservation check fails on an empty/
wrong-server index; the merger handles flaky/skipped/advisory-fail/not-run and
missing-fragment=fail.
Related to Spec 081 (release-qa-gate) T1.
…nts (Spec 081 T1) Wire the Spec 081 stage-1/2 gate driver + fixtures into CI as a single reusable workflow that qualifies a release tag before any artifact is published. ## Workflow (.github/workflows/release-qa-gate.yml) - workflow_call (publishers) + workflow_dispatch (dry run, publishes nothing — FR-001a); TEMP push trigger on the T1 branch for real CI. - build-candidate: frontend build + embed + go build of the candidate core, fixtures and release-gate driver, shared as one artifact. - suite-api-e2e: scripts/test-api-e2e.sh UNMODIFIED (FR-003). - suite-race: go test -race ./internal/... + server edition. - suite-scan-eval: scan-eval --gate --min-recall 0.90 --max-fp 0.05 on every tag regardless of changed paths (FR-015). The bare FR-003 command omits the required --corpus flag; the full eval.yml invocation is used so the gate actually runs. - matrix-invariants: five fixture upstreams (stdio/http/sse/docker/oauth, connect→list→call→kill/reconnect) + activity-request-id / counters / quarantine-flow / upgrade-in-place invariants; matrix + invariants run in one shell so the state-file'd core survives between them. - verdict (if: always()): merges fragments against the hardcoded gatereport manifest, uploads gate-report.json, exits per verdict. A missing blocking fragment is a FAIL (fail-closed, FR-004). Every job has an explicit timeout (FR-005). ## Publishers wired (FR-002) - release.yml: qa-gate job (reusable workflow) added; release.needs gains qa-gate so every public job cascading from release is gated. Guarded to the same stable-only condition so a skipped gate skips release too (never un-gates) and RC tags don't double-run the gate here. - prerelease.yml: same, guarded to prerelease TAG refs only (skips on the next branch). ## Audit (FR-022 / SC-004) - cmd/release-gate/workflow_audit_test.go parses both publisher workflows with yaml.v3 and asserts every artifact-publishing job's transitive needs closure includes the qa-gate job; statically disabled (if: false) jobs are excluded. ## Docs - docs/development/release-gate.md: what runs, how to dry-run, the T2/T3/T4 reserved extension slots + macOS-smoke promotion criterion, the FR-003 -short fallback note, and the FR-011 request-id correlation finding (X-Request-Id is not persisted on tool_call records today; correlation falls back to argument nonces + core-recorded ids).
Deploying mcpproxy-docs with
|
| Latest commit: |
3fc2729
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://310cbbf5.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://081-release-qa-gate-t1.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…ce tests, presence-check gate driver Two gate-driver defects surfaced by the TEMP push-trigger dry run: - suite-race ran the full non-short entry point (go test -race ./internal/...), which pulls in TestBinary*/TestMCP*/TestE2E_* — these hard-Fatal without a staged mcpproxy binary and exceed the 25m job budget. Fall back to the fast unit-race command proven green in e2e-tests.yml (-short -race + -skip for the binary/protocol/E2E entry points). The candidate binary is still exercised end-to-end by the server-type matrix job, and heavy variants run in e2e-tests.yml's stress job. FR-003 deviation documented in-workflow. - verdict guarded on [ ! -x dist-bin/release-gate ], but actions/upload-artifact strips the executable bit, so a successfully-built driver reads as missing and the gate self-aborts with 'candidate build failed'. Switch to a presence check ([ ! -f ]) and rely on the existing chmod +x.
The two registry-search E2E assertions proxy to the LIVE public MCP registry (/registries/official/servers). When that third-party service is slow or down the curl hits its 10s timeout and the tests hard-fail — which, under the Spec 081 tag-blocking release gate, would block every release on a third party's uptime. Add a log_skip outcome: on a confirmed external outage (curl non-zero, empty body, or upstream success:false) the test is skipped, not failed. A reachable registry returning a malformed body still hard-fails, so proxy-regression coverage is preserved. Surfaces a skipped count in the summary.
The new Spec 081 gate helpers are Linux-only: cmd/mcpfixture is built and run exclusively on the ubuntu-latest gate runner and inside the linux/amd64 Docker image, and cmd/release-gate runs only on the gate runner. Their tests exercise Unix-only semantics — exec of the built fixture + SIGTERM restart, and a copyFile owner-execute (0o100) bit — which the cross-compilation 'PR Build' Windows job cannot satisfy (exec-not-found without .exe, no Unix perm bits), turning them red. Guard them with runtime.GOOS=="windows" skips; full coverage is retained on Linux where these helpers actually run.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 28880166035 --repo smart-mcp-proxy/mcpproxy-go
|
The new release-gate docs page linked to in-repo source (workflow, cmd/release-gate, internal/gatereport, cmd/mcpfixture, scripts/gate, tests/oauthserver, spec.md, audit test) via ../../ relative paths. Docusaurus resolves those as internal site links, fails its broken-link check, and reds the Documentation build + Cloudflare Pages deploy. Point them at absolute github.com blob/tree URLs so they are treated as external links and rendered correctly.
… errors Codex review round 1 (PR #819): - checkCountersMoved skipped the telemetry sub-check whenever the AFTER snapshot was 503, masking an endpoint that regressed under matrix traffic after a healthy baseline. Skip now only when telemetry was unavailable at the boot baseline; a healthy-then-503 transition FAILS the invariant. - test-api-e2e.sh treated any registry success:false as an external outage skip. The handler returns the same writeError envelope for its own bugs, so a proxy-side regression was silently skipped. Skip now requires transport evidence (curl rc, empty body, or upstream/transport error text).
…arts them Codex review round 2 (PR #819): fixtureProc.alive() consulted Cmd.ProcessState, which is only populated by cmd.Wait. Nothing called cmd.Wait on the fixture (kill used os.Process.Wait, which does not set it), so alive() always returned true — even for a crashed/self-exited fixture, which lingered as an unreaped zombie. prepareRetry then never restarted a dead driver-owned fixture (http/sse/oauth), and retries kept hitting the dead port, defeating FR-010. Own the process with a reaper goroutine (single cmd.Wait) that closes an exited channel; alive() consults that channel and kill() waits on it instead of calling Wait a second time.
Member
Author
|
ReleaseEngineer review (MCP-4092 fallback — GeminiCritic adapter failed, reviewed directly) VERDICT: ACCEPT Verification
Architecture checkFail-closed wiring is correct:
One pre-merge actionRemove the TEMP |
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
2 participants
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.
What
Spec 081 T1 — a single tag-blocking release qualification gate. This PR is stage 3 (workflow + wiring); it lands on top of the stage-1/2 fixtures + driver already on this branch. It assembles the existing suites with a new server-type matrix and end-to-end invariants, produces one machine-readable verdict, and makes artifact publication depend on it: a red gate publishes nothing, mechanically.
Architecture
Reusable workflow
.github/workflows/release-qa-gate.yml. Every job writes a JSON report fragment into a shared dir; a finalverdictjob merges them against the hardcodedinternal/gatereportmanifest and exits per verdict.build-candidatesuite-api-e2esuite/api-e2e(scripts/test-api-e2e.shunmodified)suite-racesuite/unit-race,suite/server-racesuite-scan-evalsuite/scan-eval(unconditional, FR-015)matrix-invariantsmatrix/{stdio,http,sse,docker,oauth}+invariant/{activity-request-id,counters,quarantine-flow,upgrade-in-place}verdict(if: always())gate-report.json→$GITHUB_STEP_SUMMARY; exit code is the verdictmatrixandinvariantsrun in one shell step so the state-file'd core survives between them. Missing/failed blocking fragments fail the gate (fail-closed, FR-004). Every job has an explicit timeout (FR-005).FR coverage map
workflow_dispatchdry-run against any ref (publishes nothing).release.ymlreleasejob (and everything cascading from it) +prerelease.ymlreleasejob nowneeds:aqa-gatejob thatuses:the reusable workflow.test-api-e2e.sh,go test -race ./internal/..., server-edition race, andscan-eval --gate(not reimplementations).pass|fail|flaky|skipped|not-run|advisory-fail; no silent skips.timeout-minutesper job.cmd/release-gate/workflow_audit_test.goparses both publishers withyaml.v3and asserts every artifact-publishing job's transitiveneedsincludes theqa-gatejob (statically disabledif: falsejobs excluded). Verified it fails on an ungatedrelease.Gating safety
Both
qa-gatejobs share the exact trigger condition of theirreleasejob (release.yml: stable-only; prerelease.yml: prerelease tags only, skips thenextbranch). So a skipped gate skips the publish job too — it can never silently un-gate.workflow_dispatchofrelease.ymlis unchanged (all jobs still guard on the tag ref). RC tags don't double-run the gate inrelease.yml(prerelease.yml owns the RC gate).Notable decisions
scan-evalcommand omits the required--corpusflag; I use the fulleval.ymlinvocation (--corpus …/detect_corpus_v1.json --gate --min-recall 0.90 --max-fp 0.05) so the gate actually runs.-shortfallback:suite-raceruns the full non-short suites; if that empirically blows the 25m budget, fall back to-shortin that job only with a loud comment — the heavy property/timing tests still run unguarded ine2e-tests.yml'sstress-testsjob.X-Request-Idis not persisted ontool_callactivity records today (internal/server/mcp.gosynthesizes its own per-call ids). The check falls back to locating each call by a unique argument nonce and proving the core-recorded request id round-trips via?request_id=, and records the limitation in the fragment rather than hiding it. Persisting the caller's header end-to-end is a middleware change left out of this stage.T2–T4 extension points
Manifest reserves three non-blocking slots (recorded
not-run/not-implemented-yet):reserved/web-ui-sweep(T2 Playwright sweep vs embedded frontend),reserved/macos-smoke(T3 advisory tray smoke; promotion criterion — 3 consecutive clean tags — documented),reserved/surface-consistency(T4 REST/CLI/Web-UI/tray agreement). Seedocs/development/release-gate.md.Dry-run instructions
gh workflow run release-qa-gate.yml -f ref=<branch-or-sha>(or Actions UI → Release QA Gate → Run workflow). A dry run publishes nothing and never qualifies a later tag.Testing
go build ./...,go vet, gofmt clean.golangci-lint run --config .github/.golangci.yml ./cmd/release-gate/...→ 0 issues.go test -race ./cmd/release-gate/passes (incl. the new audit test).actionlintclean on the new workflow; no new findings on the edited publishers (pre-existing shellcheck infos untouched).yaml.safe_loadparses all three workflows.