fix(test): isolate every pass-expecting ScanForPII fixture from the runtime blocklist - #1231
Merged
Merged
Conversation
…untime 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.
Deploying mcpproxy-docs with
|
| Latest commit: |
7e13ade
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://861dbfa5.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-telemetry-blockedvalues.mcpproxy-docs.pages.dev |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 34341763996 --repo smart-mcp-proxy/mcpproxy-go
|
…t 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/
…values-shuffle-isolation # Conflicts: # cmd/mcpproxy/tools_cmd_test.go # cmd/mcpproxy/upstream_cmd_test.go
… 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.
Summary
Follow-up to #1230. That PR isolated one test from the package-global
BlockedValues, but the-shuffletail is wider: any test that callsService.Start(e.g.TestHeartbeatSend) runsPopulateBlockedValues, which appends the hostname, home-dir basename and sensitive env values toBlockedValuesfor the rest of the process. Every later pass-expectingScanForPIIcaller then fails whenever one of those values is a substring of its payload. Concretely, a home dir namedtedtripsTestScanForPII_V7FieldValidValueson"completed"(rule 2blocked_value, 3-byte minimum).Changes
withoutBlockedValues(t)helper (t.Cleanupsave/restore) to the ten un-isolated pass-expecting fixtures:TestScanForPII_CleanPayload,TestScanForPII_V7FieldValidValues,TestPayload_WizardConnectStepCompletedExternal,TestPayload_PreChurnPassesAnonymityScan,TestPayload_FunnelFieldsPopulated,TestBuildPayload_PreflightJSONRoundTrip,TestPayloadV5_DockerCLISourceIsEnumOnly,TestPreflightCounters_NoLeakPII,TestScanForPII_PreflightAllowedKeysMatchWireForm,TestScanForPII_AcceptsWellFormedPreflight.prev := BlockedValues / BlockedValues = nil / defer restorecopies with the same helper (identical semantics, one definition). Tests that expect an error, or that inject their ownBlockedValues, are untouched.TestMainwas considered and rejected:PopulateBlockedValuesis async.Oncethat fires insideService.Start, i.e. afterTestMain, so clearing there would not help.-racegate surfaced a second tail in the same family:TestNotifyConfigChanged_SendFailureStillDisablesreturned while its fire-and-forget opt-out beacon goroutine was still insideScanForPIIreadingBlockedValues, racing the next test's reset (WARNING: DATA RACE, write atwithoutBlockedValues, read atoptout.go:116). The test now points the beacon at a server that records the attempt and hijacks/drops the connection (still a transport-level send failure), and joins on that signal before returning. No production code changes.Proof the change bites
Blocklist leak,
HOME=/tmp/bite/ted, targeted pair,-shuffle=3(heartbeat test runs first):TestScanForPII_V7FieldValidValuesFAIL—rule=blocked_value pattern="ted"(anonymity_test.go:303)PASS(
TestHeartbeatSenditself fails under thatHOMEin both runs because the service correctly refuses to ship a payload containing the home-dir basename. That is the leak's source, not a target.)Beacon race,
GOMAXPROCS=1 go test -race -count=20 -shuffle=7 -run 'SendFailureStillDisables$|TrustModeDistributionAndFunnelCounters$':WARNING: DATA RACE,--- FAIL: TestPayloadV9_TrustModeDistributionAndFunnelCountersokGates
go test -race -count=1 -shuffle=on ./internal/telemetry/× 3:ok(seeds 1788936195458933000, 1788936209447958000, 1788936224100075000)/opt/homebrew/bin/golangci-lint run --config .github/.golangci.yml ./internal/telemetry/: 0 issuesFollow-up: a second, unrelated shuffle-lane failure (commit e4750fb)
The first CI run of this PR failed
Unit Tests (shuffle)incmd/mcpproxy, not ininternal/telemetry:Same class of defect, different package, and pre-existing on
main(this branch does not otherwise touchcmd/mcpproxy).TestOutputServers_InvalidFormatassignsglobalOutputFormat = "invalid-format"and never restores it. Those globals feedResolveOutputFormaton every command path, so afterwardsGetOutputFormatterfails insideoutputActivityError, which takes its early-return branch and prints the error without theHint:line.Added
setOutputFormat(t, format, jsonAlias)(saves both globals, restores viat.Cleanup) and applied it at the thirteen sites that assigned them without restoring. Sites that already saved and restored are unchanged.-shuffle=1and-shuffle=3,TestOutputServers_InvalidFormatfirstTestOutputActivityError_TableFormatHint:Gates re-run over both packages:
go test -race -shuffle=on -count=1 ./cmd/mcpproxy/ ./internal/telemetry/× 3 allok; golangci-lint v2 over both, 0 issues.Note:
cmd/mcpproxy/activity_cmd_test.gois unformatted onmain(a struct-field alignment block at ~line 861). It is untouched here to keep this diff to the fix.Merge with
main: thecmd/mcpproxyhalf was fixed upstream first#1233 landed the same
cmd/mcpproxyfix onmainwhile this PR was open, under the helper namesetOutputGlobals(identical save/restore-via-t.Cleanupsemantics, plus a self-test). Mergedorigin/mainin and resolved both conflicted files to main's version, then deleted this branch's now-duplicatesetOutputFormathelper. Commit e4750fb is therefore a no-op against currentmain, and the surviving diff isinternal/telemetryonly.Re-audited
cmd/mcpproxyafter the resolution: no test assigns the output globals without restoring them. Gates re-run on the merged tree —go test -race -shuffle=on -count=1 ./cmd/mcpproxy/ ./internal/telemetry/twice, bothok; golangci-lint v2 over both packages, 0 issues.Cross-model review (opencode,
github-copilot/gpt-6-astra)Reviewed in two file-named chunks.
Chunk 1,
optout_test.go(the beacon join). Approved for the race fix, with one accuracy finding and one defensive note, both applied in 7e13ade:attemptedwaits until the goroutine has reached the HTTP send, which is past itsScanForPIIread ofBlockedValues; nothing it does after the send touches the blocklist. Both comments now claim only that.Close. Not an exercised path, but the signal is now a non-blockingselect.Hijack-unsupported fallback returns 500, whichSendOptOutBeaconalso treats as an error.Chunk 2, the ten isolation files. Approved with no findings. Specifically confirmed: no pass-expecting
ScanForPIIcaller was missed; no error-expecting test or deliberate-blocklist test was weakened (TestScanForPII_RawMachineIDBlockedandTestScanForPII_BlockedValue_EnvVarkeep their own injected values, andTestPopulateBlockedValuesFromstill clears before injecting); and everydefertot.Cleanupconversion is at top-level test scope, so parent cleanup runs after all subtests and table iterations.Gates re-run after the fixes:
go test -race -shuffle=on -count=1 ./cmd/mcpproxy/ ./internal/telemetry/× 3 allok; theGOMAXPROCS=1 -count=20race stressok; golangci-lint v2 over both packages, 0 issues.