ci: add an advisory go test -shuffle=on lane so order-coupled tests fail in CI - #1230
Merged
Conversation
…ail in CI The registry SSRF regression test only passed because its file sorted ahead of the two fixtures that flipped the process-global allow-policy (#1222). Nothing in .github/workflows/ ran with -shuffle, so no lane could see a different order. Add 'Unit Tests (shuffle)': the required ubuntu lane's flags and provisioning (skip regex, -race, tscg shim, frontend embed, tiktoken warm-up) plus -shuffle=on, without coverage. It is not in the branch-protection required list, so a newly exposed order coupling in an unrelated package cannot block a PR while the tail of such tests is found. Each package prints its seed as '-test.shuffle <n>'; re-run with -count=1 -shuffle=<n> to reproduce.
…lockedValues Service.Start reaches PopulateBlockedValues, a sync.Once that appends the real hostname and home-dir basename to the package-global BlockedValues and is never undone. Five test files call Start, and this was the one ScanForPII test that did not snapshot the global, so under -shuffle a basename such as "user" tripped rule 2 (blocked_value) on the "terminated by user" payload before rule 7 could report v7_field_invalid. Found by the new shuffle lane's first local run (-shuffle=1788926312892506000). Same save-and-restore the neighbouring ScanForPII tests already use.
Deploying mcpproxy-docs with
|
| Latest commit: |
63249ac
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://86893fb9.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://ci-shuffle-lane.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 34309799091 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
-shuffle is not a cacheable go test flag, so a seed replay is never served from the result cache; the note claimed the opposite. Reword to say why -count=1 is still kept and what the seed does and does not reproduce.
This was referenced Sep 9, 2026
Dumbris
added a commit
that referenced
this pull request
Sep 9, 2026
…them (#1233) The shuffle lane added in #1230 went red on its first unrelated PR, which is what it is for. globalOutputFormat and globalJSONOutput are package-level flag targets in cmd/mcpproxy. Fifteen tests assigned them and never restored, so whichever ran last left its choice installed for the rest of the binary. In file order that is invisible. Under -shuffle=on it is not: TestVersionCommandTableOutput and TestOutputActivityError_TableFormat both render through clioutput.ResolveFormat and assert the default table output, so an inherited "json" fails them. Seed 1788937037978355714 reproduces both on origin/main. Adds setOutputGlobals(t, format, jsonOut), which saves the pair and restores it from t.Cleanup, and routes all fifteen sites through it. The sites that already saved and restored by hand are left alone. No production code changes, and no test's intended format changes. Verified: the CI seed fails on main and passes here; five fresh -race -shuffle=on runs of the package are green; golangci-lint v2 with the CI config reports 0 issues. Note for a follow-up, not touched here: cmd/mcpproxy/activity_cmd_test.go is already unformatted on origin/main.
Dumbris
added a commit
that referenced
this pull request
Sep 9, 2026
…untime blocklist (#1231) * fix(test): isolate every pass-expecting ScanForPII fixture from the runtime blocklist PR #1230 isolated TestScanForPII_V7FieldViolations from the package-global BlockedValues, but the -shuffle tail was wider: any test that calls Service.Start (TestHeartbeatSend and friends) runs PopulateBlockedValues, which appends the hostname, home-dir basename and sensitive env values to BlockedValues for the rest of the process. Every later ScanForPII caller that expects a clean verdict then fails whenever one of those values is a substring of its payload (a home dir named "ted" trips on "completed"). Apply the existing withoutBlockedValues(t) helper to the ten un-isolated pass-expecting fixtures and replace the seven hand-rolled save/nil/defer-restore copies with the same helper. Tests that expect an error, or that inject their own BlockedValues, are untouched. A package TestMain would not work here: PopulateBlockedValues is a sync.Once that fires inside Service.Start, i.e. after TestMain has run. The -race gate also surfaced a second order-coupled tail in the same family: TestNotifyConfigChanged_SendFailureStillDisables returned while its fire-and-forget opt-out beacon goroutine was still inside ScanForPII reading BlockedValues, racing the next test's reset. The test now points the beacon at a server that records the attempt and drops the connection (still a transport-level send failure), and joins on that signal before returning. * fix(test): restore the CLI output-format globals after every test that sets them The advisory shuffle lane failed on TestOutputActivityError_TableFormat with `"Error: test error message\n" does not contain "Hint:"`. Root cause is in a different test: TestOutputServers_InvalidFormat assigns globalOutputFormat = "invalid-format" and never restores it. Those globals feed ResolveOutputFormat on every command path, so once one test leaves them dirty GetOutputFormatter fails inside outputActivityError, which takes its early-return branch and prints the error without the Hint line. Add setOutputFormat(t, format, jsonAlias), which saves both globals and restores them via t.Cleanup, and use it at the thirteen sites that assigned them without restoring. The sites that already saved and restored are unchanged. Pre-existing on main and unrelated to the telemetry change in this branch; it is fixed here because it blocks this PR's shuffle lane. Reproduces before the fix and passes after, same seeds: go test -count=1 -shuffle=1 (and 3) \ -run 'TestOutputServers_InvalidFormat$|TestOutputActivityError_TableFormat$' \ ./cmd/mcpproxy/ * fix(test): say what the beacon wait actually guarantees, and make the signal non-blocking Cross-model review (opencode, gpt-6-astra) approved the change but caught the comment overstating it: receiving from `attempted` waits until the beacon goroutine has reached the HTTP send, which is past its ScanForPII read of BlockedValues. It does not join the goroutine, and nothing the goroutine does after the send touches the blocklist. Reworded both comments to claim only that. Also made the handler's signal a non-blocking select. The capacity-one channel is only received once, so a retried or duplicated request would otherwise wedge the handler and hang the deferred server Close. Not an exercised path today, but free to rule out. No behaviour change to what the test proves: the connection is still hijacked and dropped without a response, so the send still fails at the transport level.
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.
Motivation
#1222 (@loloDawit) fixed two
internal/serverfixtures that flipped the process-global registry SSRF allow-policy and never restored it.TestBuildRegistrySourceEntry_RejectsSSRFLiteralIP— the CWE-918 regression test — only passed because its file sorted ahead of both fixtures. The PR closed with:This is that lane. Nothing under
.github/workflows/ran with-shufflebefore, so no CI job could ever see an order other than file/declaration order.What the lane runs
A new job
Unit Tests (shuffle)inunit-tests.yml, ubuntu-latest only:That is the required ubuntu lane's invocation with one flag added (
-shuffle=on) and one dropped (-coverprofile— Codecov already gets the ordered run). It reuses the same provisioning the required job needs forgo test ./...: Go 1.25 with module cache, the pinned TSCG Node shim (npm ci --prefix bench/tscg, Spec 083 FR-006 hard-fails without it), thefrontend-distartifact copied intoweb/frontend/for the embed, and thewarmtiktokenpre-step that removes the tiktoken download rename race. The requiredUnit Testsjob is unchanged.Why advisory first
The job is deliberately not in the branch-protection required list (
Unit Tests (ubuntu-latest, 1.25),Lint,Build (ubuntu-latest), … —gh api .../branches/main/protection/required_status_checks), so a newly exposed order coupling in an unrelated package cannot block a PR while the tail of such tests is being found. It has nocontinue-on-error: a failure shows as a red check on the PR so it gets looked at, it just cannot block the merge.Each CI run draws a fresh seed, so exposure accumulates across runs rather than re-testing one order forever.
Proposal: promote it to required after 10 consecutive green runs on
main.Reproducing a failure from the log
With
-shuffleon, every test binary prints its seed as the first line of the package's output:Re-run the failing package with that exact seed:
-count=1matters — the seed reproduces the order, not the cache, and a cachedokwould hide the failure. Add the-skipregex from the workflow if the failing test lives next to E2E tests. The workflow carries this recipe as a comment above the job.Local run
on this branch's base (
a742ef75d): 64 packages ok, 1 FAIL —internal/telemetry, seed1788926312892506000:Same class as #1222, different global.
Service.StartcallsPopulateBlockedValues(), async.Oncethat appends the real hostname and home-dir basename to the package-globalBlockedValuesand is never undone. Five test files callStart. Every otherScanForPIItest inanonymity_test.gosnapshotsBlockedValuesand restores it;TestScanForPII_V7FieldViolationswas the one that did not, so when aStarttest runs first, a home basename such asusertrips rule 2 (blocked_value) on the"terminated by user"payload before rule 7 can reportv7_field_invalid.Fixed here because it is trivially the #1222 shape — three lines of save/nil/restore, identical to the neighbouring tests in the same file. Verified: the reproducing seed fails on the base commit and passes with the fix; ordered run and seeds 1, 2, 3 all pass;
golangci-lintv2 with.github/.golangci.ymlreports 0 issues on the package.Note this one is environment-dependent: it bites only when the runner's home basename or hostname is a substring of a test payload, so CI (
/home/runner) may never have hit it, and a laptop with a different login may not either. The seed reproduces it on a machine wherebasename $HOMEisuser.Ordered run:
TestGetServerLogs_MissingFileReturnsEmptyNotError, which #1222 flagged as a pre-existing-count=2failure, was fixed in #1228 and did not fail here.Follow-ups
./...(bench, cmd, …), while the local run covered./internal/...only, so the first few seeds may surface more.main, addUnit Tests (shuffle)to the required checks.Testing
actionlint .github/workflows/unit-tests.yml— clean./internal/...— one order coupling found, fixed here, re-verified against the reproducing seedgolangci-lintv2 (.github/.golangci.yml) on the changed package — 0 issues