test(mcp): rewrite GlobalScope_Blocked to assert OFFSEC-001 scrub contract (mc#664 Class 2)

Background — chain of defects
-----------------------------
mc#664 (Platform (Go) CI red) decomposes into:
  • Class 1 — 4 TestExecuteDelegation_* failures (parallel dispatch to core-be)
  • Class 2 — TestMCPHandler_CommitMemory_GlobalScope_Blocked (this PR)

Class 2 root cause: commit 7d1a189f (2026-05-10) hardened mcp.go to scrub
err.Error() out of the JSON-RPC error.message returned to the client,
replacing the third leak (the dispatchRPC tool-call branch, line ~427)
with the constant string "tool call failed". The internal error is now
log.Printf'd server-side only.

The existing test at mcp_test.go:432 asserted that the client-visible
message CONTAINED the substring "GLOBAL" — which was exactly the
internal err.Error() text the 7d1a189f scrub now removes. So the test
had silently flipped from "verifies behaviour" to "verifies the bug",
and once the scrub landed the test went red. PR #665 has been masking
this red via continue-on-error as an interim measure; this PR is the
proper fix for Class 2.

Wrong fix
---------
Un-scrub mcp.go (i.e. restore err.Error() into the client-facing
message). This would re-open OFFSEC-001 / #259 and defeat the security
hardening that was applied uniformly across 22 sibling files in
PRs #1193 / #1206 / #1219 / #168.

Right fix (this PR)
-------------------
Rewrite the test so it asserts the OFFSEC-001 scrub-works contract
on this very code path, matching the same style used by the four
canonical OFFSEC-001 tests already in this file (lines 1031–1149):

  • exact-equality on resp.Error.Code (-32000)
  • exact-equality on resp.Error.Message ("tool call failed")
  • negative-substring canaries on six tokens from the production-internal
    error string ("GLOBAL", "scope", "permitted", "bridge", "LOCAL", "TEAM")
    — if ANY leaks through to the client, the scrub has regressed and the
    test fires immediately
  • C3 invariant preserved (no DB calls — handler short-circuits)
  • Test renamed to _ScrubsInternalError so the contract is visible at
    the call site / in failure output

Per feedback_assert_exact_not_substring: the positive assertion uses
exact-equality (`!= "tool call failed"`) rather than substring-match,
so any future mutation of the constant breaks the test loudly.

Verification (local, falsified both ways)
-----------------------------------------
  Positive: against current main (7d1a189f scrub in place)
    $ go test -run TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError
    ok      .../internal/handlers   0.515s  PASS

  Falsification: temporarily reverted line 427 of mcp.go to
  `Message: err.Error()`, ran the test → all positive assertions failed
  AND all six leaked-token canaries fired (proves the test really does
  guard the contract, not just shape).

All other TestMCPHandler_* tests continue to pass. The four
TestExecuteDelegation_* failures observed in the full handlers/
package run pre-exist on origin/main and are Class 1 (core-be's
parallel work) — not touched here.

Tier
----
tier:high — this is the security-hardening contract test for the
OFFSEC-001 scrub. A weak version of this assertion is what allowed
the original gap on the GLOBAL-scope path to go unnoticed for so long.

Brief-falsification log
-----------------------
  • Brief halt-condition: "If reading of 7d1a189f differs from this
    brief's account: STOP" — confirmed identical (3rd hunk, line 425 in
    pre-patch mcp.go, dispatchRPC tool-call branch, scrubs err.Error()
    → "tool call failed", logs server-side).
  • Brief halt-condition: "If mcp_test.go line 433 has been modified
    since this brief was written: STOP" — confirmed unchanged
    (line 432–434 exact text matches brief description).
  • Brief widen-scope check: searched for sibling tests with the same
    anti-pattern (assert internal err.Error() content on the OFFSEC
    code path). Findings:
      – TestMCPHandler_RecallMemory_GlobalScope_Blocked (line 539)
        asserts `resp.Error != nil` only; does NOT assert on
        "GLOBAL"-substring, so it isn't broken by the scrub. BUT it
        also doesn't verify the scrub-works contract — a future
        regression would slip past it. Recommending a follow-up to
        strengthen it (and the corresponding RecallMemory v2 path,
        if any) in a separate single-purpose PR rather than widening
        scope here. NOT addressed in this PR per the brief's
        "1-2 siblings or report" discipline.
  • OFFSEC-001 issue lookup: 22 files were touched by the sibling
    scrub PRs (#1193 / #1206 / #1219 / #168). This PR addresses ONE
    test that was asserting against the now-scrubbed surface. No
    other red-on-main tests are believed to share this anti-pattern
    in mcp_test.go (grep verified).

References
----------
  • mc#664 (Platform (Go) red — chain root issue)
  • PR #665 (interim continue-on-error mask — to be reverted post-fix)
  • commit 7d1a189f (OFFSEC-001 scrub, the hardening this test now guards)
  • OFFSEC-001 / molecule-ai/molecule-core#259 (original security issue)
  • feedback_assert_exact_not_substring (assertion-style memory)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

workspace-server/internal/handlers/mcp_test.go

@@ -417,11 +417,32 @@ func TestMCPHandler_CommitMemory_LocalScope_Success(t *testing.T) {
 	}
 }
 
-// TestMCPHandler_CommitMemory_GlobalScope_Blocked verifies that C3 is enforced:
-// GLOBAL scope is not permitted on the MCP bridge.
-func TestMCPHandler_CommitMemory_GlobalScope_Blocked(t *testing.T) {
+// TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError verifies
+// two contracts at once on the GLOBAL-scope-blocked path:
+//
+//  1. C3 invariant (commit_memory with scope=GLOBAL aborts on the MCP bridge
+//     before touching the DB), AND
+//  2. OFFSEC-001 / #259 scrub contract (commit 7d1a189f): the JSON-RPC error
+//     returned to the client is a CONSTANT — code=-32000, message="tool call
+//     failed" — with the production-internal err.Error() text logged
+//     server-side, never reflected back to the caller.
+//
+// Prior to this rename the test asserted that the client-visible message
+// CONTAINED the substring "GLOBAL", which was the human-readable internal
+// error from toolCommitMemory. mc#664 Class 2 flipped that assertion the
+// right way around: now the test FAILS if the scrub regresses (i.e. if the
+// internal string is ever reflected back to the wire), and PASSES iff the
+// scrubbed constant reaches the client.
+//
+// Coupling note: the constant string "tool call failed" and the code -32000
+// are the same values asserted by
+// TestMCPHandler_dispatchRPC_UnknownTool_ReturnsConstantMessage — both are
+// the OFFSEC-001 contract for the dispatch-failure branch in mcp.go (the
+// third err.Error() leak that 7d1a189f scrubbed). If those constants ever
+// change, both tests must move together.
+func TestMCPHandler_CommitMemory_GlobalScope_Blocked_ScrubsInternalError(t *testing.T) {
 	h, mock := newMCPHandler(t)
-	// No DB expectations — handler must abort before touching the DB.
+	// No DB expectations — handler must abort before touching the DB (C3).
 
 	w := mcpPost(t, h, "ws-1", map[string]interface{}{
 		"jsonrpc": "2.0",
@@ -436,14 +457,53 @@ func TestMCPHandler_CommitMemory_GlobalScope_Blocked(t *testing.T) {
 		},
 	})
 
+	// JSON-RPC envelope returns 200 with the error in the body — only
+	// malformed-JSON-at-the-envelope-layer returns 400 (see Call() in mcp.go).
+	if w.Code != http.StatusOK {
+		t.Fatalf("expected 200 (JSON-RPC error in body), got %d: %s", w.Code, w.Body.String())
+	}
+
 	var resp mcpResponse
-	json.Unmarshal(w.Body.Bytes(), &resp)
+	if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil {
+		t.Fatalf("response is not valid JSON: %v", err)
+	}
+
+	// (1) C3: an error must be reported.
 	if resp.Error == nil {
-		t.Error("expected JSON-RPC error for GLOBAL scope, got nil")
+		t.Fatal("expected JSON-RPC error for GLOBAL scope, got nil")
 	}
-	if resp.Error != nil && !bytes.Contains([]byte(resp.Error.Message), []byte("GLOBAL")) {
-		t.Errorf("error message should mention GLOBAL, got: %s", resp.Error.Message)
+
+	// (2) OFFSEC-001 positive assertions — exact equality on the scrubbed
+	// constants so any change (re-leak of err.Error(), code mutation) trips
+	// the test. Substring-match would not catch a partial re-leak.
+	if resp.Error.Code != -32000 {
+		t.Errorf("error code should be -32000 (Server error / dispatch-failure), got: %d", resp.Error.Code)
 	}
+	if resp.Error.Message != "tool call failed" {
+		t.Errorf("error message should be the OFFSEC-001 constant %q, got: %q", "tool call failed", resp.Error.Message)
+	}
+
+	// (3) OFFSEC-001 negative assertions — the internal err.Error() text
+	// from toolCommitMemory ("GLOBAL scope is not permitted via the MCP
+	// bridge — use LOCAL or TEAM") must NOT appear in the client-visible
+	// message. Each token below is a distinct substring of that internal
+	// string; if ANY leaks through, the scrub in mcp.go dispatchRPC has
+	// regressed and this assertion fires the canary.
+	leakedTokens := []string{
+		"GLOBAL",    // scope name
+		"scope",     // policy lexicon
+		"permitted", // policy verb
+		"bridge",    // internal architecture term
+		"LOCAL",     // alternative scope name
+		"TEAM",      // alternative scope name
+	}
+	for _, tok := range leakedTokens {
+		if bytes.Contains([]byte(resp.Error.Message), []byte(tok)) {
+			t.Errorf("OFFSEC-001 scrub regression: client-visible error.message leaks internal token %q (got: %q)", tok, resp.Error.Message)
+		}
+	}
+
+	// (4) C3 invariant preserved: handler must short-circuit before any DB call.
 	if err := mock.ExpectationsWereMet(); err != nil {
 		t.Errorf("unexpected DB calls on GLOBAL scope block: %v", err)
 	}