fix(scope): direct-surface definitions authorized by producing identity at every seam (Spec 105 PR F, FR-008)

ROADMAP.md

@@ -888,7 +888,7 @@ graph LR
 | Web UI + macOS app UX audit | In progress | P0 | — |  |  |
 | Release qualification gate (auto-QA matrix blocks the tag) | In progress | P0 | — | [081-release-qa-gate](./specs/081-release-qa-gate/) |  |
 | Action log / transparency — info at a glance | In progress | P1 | — |  |  |
-| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 53/110 (48%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) |  |
+| Agent-token scope hardening: every MCP request authorized by its own scope (spec 105) | In progress | P1 | 68/111 (61%) | [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) |  |
 | Token-efficiency benchmark: measured savings, published results | In progress | P1 | 62/64 (97%) | [103-token-bench](./specs/103-token-bench/) |  |
 | Telemetry identity & data quality (machine_id + CI-filter hardening) | In progress | P1 | — |  |  |
 | Telemetry v7: honest funnel + churn instrumentation | In progress | P1 | — | [080-telemetry-v7-churn](./specs/080-telemetry-v7-churn/) |  |
@@ -1034,6 +1034,6 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—`
 | [102-schema-deferred](./specs/102-schema-deferred/) | `shipped` | 89/89 (100%) |
 | [103-token-bench](./specs/103-token-bench/) | `shipped` | 62/64 (97%) |
 | [104-auto-routing-mode](./specs/104-auto-routing-mode/) | — | — |
-| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 53/110 (48%) |
+| [105-agent-scope-hardening](./specs/105-agent-scope-hardening/) | `in-flight` | 68/111 (61%) |
 | [106-security-residual-fixes](./specs/106-security-residual-fixes/) | `shipped` | 18/19 (95%) |
 | [107-server-edition-sso-hardening](./specs/107-server-edition-sso-hardening/) | `shipped` | 126/126 (100%) |

internal/server/mcp_direct_callability.go

@@ -59,28 +59,44 @@ func (p *MCPProxyServer) filterDirectToolsForAgentCallability(ctx context.Contex
 	evaluator := newDirectCallabilityEvaluator(p)
 	filtered := make([]mcp.Tool, 0, len(tools))
 	for _, tool := range tools {
-		// Same catalog resolution as filterDirectModeToolsForAuth (D10). The two
-		// filters run over the same listing, so if they resolved names
-		// differently — one by catalog, one by first-"__" parse — a server whose
-		// name contains "__" could be scope-checked as one origin and
-		// callability-checked as another.
-		entry, decision := p.resolveDirectTool(tool.Name)
-
 		var serverName, toolName string
-		switch decision {
-		case directResolveBuiltin:
-			// Built-ins are this proxy's own tools; there is no upstream
-			// approval record to evaluate.
-			filtered = append(filtered, tool)
-			continue
-		case directResolveDenied:
-			continue
-		case directResolveNoCatalog:
-			// The parse cannot fail: a separator-less name was already classified
-			// as a built-in above.
-			serverName, toolName, _ = ParseDirectToolName(tool.Name)
-		case directResolveFound:
-			serverName, toolName = entry.ServerName, entry.ToolName
+
+		if stamp, stamped := readDirectToolStamp(tool); stamped {
+			// Spec 105 FR-008: the identity STAMPED on this exact tool object,
+			// never re-derived from a fresh catalog lookup (see
+			// directToolStamp's doc comment).
+			if stamp.rawName == "" {
+				continue
+			}
+			serverName, toolName = stamp.owner, stamp.rawName
+		} else {
+			// No stamp: fall back to the pre-105 catalog/builtin resolution,
+			// exactly as filterDirectModeToolsForAuth does. Same catalog
+			// resolution the scope filter uses (D10): the two filters run over
+			// the same listing, so if they resolved names differently — one by
+			// catalog, one by first-"__" parse — a server whose name contains
+			// "__" could be scope-checked as one origin and
+			// callability-checked as another.
+			//
+			// Same residual as filterDirectModeToolsForAuth's fallback branch
+			// (see its doc comment): an unstamped mcp-go SESSION tool sharing a
+			// global tool's name would resolve here too. Not reachable today —
+			// mcpproxy-go registers no session-specific tools on this surface.
+			entry, decision := p.resolveDirectTool(tool.Name)
+
+			switch decision {
+			case directResolveBuiltin:
+				// Built-ins are this proxy's own tools; there is no upstream
+				// approval record to evaluate.
+				filtered = append(filtered, tool)
+				continue
+			case directResolveDenied:
+				continue
+			case directResolveNoCatalog:
+				serverName, toolName, _ = ParseDirectToolName(tool.Name)
+			case directResolveFound:
+				serverName, toolName = entry.ServerName, entry.ToolName
+			}
 		}
 
 		if evaluator.evaluate(serverName, toolName).callable {
@@ -141,17 +157,62 @@ func (p *MCPProxyServer) directToolCallabilityBlockWithReason(ctx context.Contex
 	return p.directToolCallabilityResult(ctx, decision, args), directBlockReasonKey(decision)
 }
 
+// directRefusalKind is the ONE precedence order a direct-mode callability
+// block resolves to, shared by directBlockReasonKey (telemetry) and
+// directToolCallabilityResult (the response body) so the two can never
+// disagree about which gate "fired" for a tool that trips more than one at
+// once (PR #1326 review round 2, chunk B: a tool can be BOTH config-denied
+// AND pending/changed approval at the same time, and the two functions used
+// to classify that case differently — the response said config-denied, the
+// telemetry said pending).
+//
+// The order mirrors the established dispatch precedence every OTHER path
+// already uses (handleCallToolVariant / handleCallTool in mcp.go, via
+// toolGate.lockStatus): quarantine, then the approval lock (pending/changed),
+// then the generic/config-denied block. Direct mode's RESPONSE function had
+// drifted from that precedence (config-denied was checked before the
+// approval lock); this converges it rather than inventing a third order.
+type directRefusalKind int
+
+const (
+	directRefusalNone directRefusalKind = iota
+	directRefusalQuarantined
+	directRefusalPending
+	directRefusalChanged
+	directRefusalConfigDenied
+	directRefusalGeneric
+)
+
+// classifyDirectRefusal is the single source of truth for which refusal a
+// blocked directCallabilityDecision represents. Both directBlockReasonKey and
+// directToolCallabilityResult switch on its result instead of re-deriving
+// their own branch order, so they cannot drift apart again.
+func classifyDirectRefusal(decision directCallabilityDecision) directRefusalKind {
+	switch {
+	case decision.serverConfig != nil && decision.serverConfig.Quarantined:
+		return directRefusalQuarantined
+	case decision.approvalStatus == storage.ToolApprovalStatusPending:
+		return directRefusalPending
+	case decision.approvalStatus == storage.ToolApprovalStatusChanged:
+		return directRefusalChanged
+	case decision.configDenied:
+		return directRefusalConfigDenied
+	default:
+		return directRefusalGeneric
+	}
+}
+
 // directBlockReasonKey classifies a direct-mode callability block onto the
-// closed telemetry.BlockReason* enum. The branches mirror
-// directToolCallabilityResult exactly, so the counted reason always matches the
+// closed telemetry.BlockReason* enum, from the SAME classification
+// directToolCallabilityResult uses, so the counted reason always matches the
 // payload the caller was handed.
 func directBlockReasonKey(decision directCallabilityDecision) string {
-	switch {
-	case decision.serverConfig != nil && decision.serverConfig.Quarantined:
+	switch classifyDirectRefusal(decision) {
+	case directRefusalQuarantined:
 		return telemetry.BlockReasonServerQuarantined
-	case decision.approvalStatus == storage.ToolApprovalStatusPending:
+	case directRefusalPending:
 		return telemetry.BlockReasonToolPendingApproval
-	case decision.approvalStatus == storage.ToolApprovalStatusChanged:
+	case directRefusalChanged:
 		return telemetry.BlockReasonToolChanged
 	default:
 		// Disabled server, config-denied tool, per-tool disable, and the
@@ -296,22 +357,16 @@ func (e *directCallabilityEvaluator) getToolApproval(serverName, toolName string
 }
 
 func (p *MCPProxyServer) directToolCallabilityResult(ctx context.Context, decision directCallabilityDecision, args map[string]interface{}) *mcp.CallToolResult {
-	if decision.serverConfig != nil && decision.serverConfig.Quarantined {
+	switch classifyDirectRefusal(decision) {
+	case directRefusalQuarantined:
 		return p.handleQuarantinedToolCall(ctx, decision.serverName, decision.toolName, args)
-	}
-
-	if decision.configDenied {
+	case directRefusalPending:
+		return toolPendingApprovalResult(decision.serverName, decision.toolName, decision.approval)
+	case directRefusalChanged:
+		return toolChangedApprovalResult(decision.serverName, decision.toolName, decision.approval)
+	case directRefusalConfigDenied:
 		return mcp.NewToolResultError(blockedToolMessageFor(true))
+	default:
+		return mcp.NewToolResultError(p.blockedToolMessage(decision.serverName, decision.toolName))
 	}
-
-	if decision.approval != nil {
-		switch decision.approvalStatus {
-		case storage.ToolApprovalStatusPending:
-			return toolPendingApprovalResult(decision.serverName, decision.toolName, decision.approval)
-		case storage.ToolApprovalStatusChanged:
-			return toolChangedApprovalResult(decision.serverName, decision.toolName, decision.approval)
-		}
-	}
-
-	return mcp.NewToolResultError(p.blockedToolMessage(decision.serverName, decision.toolName))
 }

internal/server/mcp_direct_callability_test.go

@@ -55,6 +55,23 @@ func TestDirectToolCallabilityBlock_ConfigDeniedTool(t *testing.T) {
 		Enabled:       true,
 		DisabledTools: []string{"delete_repo"},
 	}))
+	// A pre-existing APPROVED record isolates this test's target: the tool
+	// case (config-denied) from the approval-lock gate. Without one, a fresh
+	// direct-mode evaluation with no approval record at all synthesizes an
+	// implicit "pending" record while the quarantine gate is active (Spec 105
+	// FR-009), and — per PR #1326 review round 2 finding #1 — the approval
+	// lock now correctly wins over a plain config denial, matching the
+	// established handleCallToolVariant/handleCallTool precedence
+	// (toolGate.lockStatus checked before the generic/config-denied block).
+	// That combined scenario is covered by
+	// TestDirectBlockReasonKey_AgreesWithResponse_ConfigDeniedAndApprovalLocked
+	// in preflight_telemetry_test.go; this test isolates the config-denied
+	// response body in the case that ambiguity does not arise.
+	require.NoError(t, proxy.storage.SaveToolApproval(&storage.ToolApprovalRecord{
+		ServerName: "github",
+		ToolName:   "delete_repo",
+		Status:     storage.ToolApprovalStatusApproved,
+	}))
 
 	result := proxy.directToolCallabilityBlock(context.Background(), "github", "delete_repo", map[string]interface{}{})
 	require.NotNil(t, result)

internal/server/mcp_direct_catalog.go

@@ -166,6 +166,20 @@ func buildDirectCatalog(tools []*config.ToolMetadata, logger *zap.Logger) *direc
 		if t == nil {
 			continue
 		}
+		if t.Name == "" {
+			// Spec 105 FR-008 (FR008-G7): an upstream tool with an empty raw
+			// name renders as "server__" and has no registration identity to
+			// authorize it against — the direct-surface analogue of the
+			// FR-006 empty-prompt-name rule. Refused admission here, at the
+			// source, rather than admitted and relied on to be caught by a
+			// downstream filter: withheld from every caller, administrators
+			// included (SC-005 exception).
+			if logger != nil {
+				logger.Warn("Withholding direct tool with an empty raw name: no registration identity to authorize it against",
+					zap.String("server_name", t.ServerName))
+			}
+			continue
+		}
 		name := FormatDirectToolName(t.ServerName, t.Name)
 		if _, seen := grouped[name]; !seen {
 			order = append(order, name)
@@ -367,12 +381,6 @@ const (
 	directResolveNoCatalog
 )
 
-// builtinDirectToolNames is an explicit allowlist for built-ins whose display
-// name WOULD parse as server__tool and so cannot be recognised structurally.
-// Empty today; it exists so adding such a built-in is a deliberate act rather
-// than an accidental denial.
-var builtinDirectToolNames = map[string]struct{}{}
-
 // resolveDirectTool maps a direct display name to its catalog entry.
 //
 // This replaces ParseDirectToolName as the resolution path for the discovery
@@ -393,30 +401,26 @@ func (p *MCPProxyServer) resolveDirectTool(displayName string) (*directCatalogEn
 	// through the scope, tier and callability gates like any other.
 	//
 	// This ordering is load-bearing, and getting it wrong was a real disclosure
-	// bug. The structural test below assumes every upstream display name parses,
-	// because FormatDirectToolName always inserts "__". It does not: an upstream
-	// tool whose NAME IS EMPTY renders as "server__", which ParseDirectToolName
-	// rejects (the tool half is empty). That name was therefore classified as a
-	// proxy built-in, and both direct filters pass built-ins through
-	// unconditionally — so an agent token scoped to other servers could see the
-	// name, description and annotations of a tool on a server outside its scope.
-	// Found by adversarial QA, not by any unit test, because no fixture had ever
+	// bug: an upstream tool whose NAME IS EMPTY renders as "server__", which
+	// ParseDirectToolName rejects (the tool half is empty). A name with no
+	// "__" separator was therefore once inferred a proxy built-in structurally
+	// — and both direct filters pass built-ins through unconditionally — so an
+	// agent token scoped to other servers could see the name, description and
+	// annotations of a tool on a server outside its scope. Found by
+	// adversarial QA, not by any unit test, because no fixture had ever
 	// contained a nameless tool.
+	//
+	// Spec 105 FR-008 (FR008-G7) closes this at its source: buildDirectCatalog
+	// now refuses to admit an entry with an empty raw tool name at all, so
+	// "server__" is never in this catalog to begin with, and the structural
+	// "no separator -> built-in" inference below is gone entirely — a name is
+	// a built-in ONLY via the explicit builtinDirectToolNames set checked
+	// above. A name that is neither stamped in the catalog nor a recognised
+	// built-in has no registration identity and falls through to denial.
 	if entry, ok := cat.Lookup(displayName); ok {
 		return entry, directResolveFound
 	}
 
-	// A name with no "__" separator that the catalog does NOT admit is something
-	// this proxy registered itself — describe_tool, retrieve_tools on a shared
-	// surface — and denying it would delete built-ins off their own surface.
-	//
-	// This is the structural half of D13 rule 2's "built-ins by explicit name
-	// set". The set above covers the residual case a structural test cannot: a
-	// built-in whose name happens to contain "__".
-	if _, _, ok := ParseDirectToolName(displayName); !ok {
-		return nil, directResolveBuiltin
-	}
-
 	if cat == nil {
 		return nil, directResolveNoCatalog
 	}

internal/server/mcp_direct_catalog_publish_test.go

@@ -89,16 +89,27 @@ func TestResolveDirectTool_DenyOnMissButNotOnNilCatalog(t *testing.T) {
 			"a name the catalog does not admit must be denied, not waved through by re-parsing it")
 	})
 
-	t.Run("a separator-less name is a built-in, not a denial", func(t *testing.T) {
-		// Every upstream tool is named through FormatDirectToolName, which always
-		// inserts "__". A name without one therefore cannot be an upstream
-		// projection — it is a tool this proxy registered itself, and denying it
-		// would delete built-ins off their own surface.
-		for _, name := range []string{"describe_tool", "retrieve_tools"} {
-			entry, decision := p.resolveDirectTool(name)
-			assert.Nil(t, entry, "a built-in has no upstream catalog entry")
-			assert.Equal(t, directResolveBuiltin, decision, "%s must be kept", name)
-		}
+	t.Run("a separator-less name is a built-in ONLY via the explicit set", func(t *testing.T) {
+		// Spec 105 FR-008 (FR008-G2): "built-in" used to be inferred
+		// structurally — any name that fails to parse as server__tool — which
+		// misclassified an upstream tool with an empty raw name ("server__")
+		// the same way (TestResolveDirectTool_EmptyToolNameIsNotABuiltin).
+		// Positive identification replaces it: a name is a built-in only when
+		// it is in builtinDirectToolNames, populated from this surface's own
+		// constructors.
+		entry, decision := p.resolveDirectTool("describe_tool")
+		assert.Nil(t, entry, "a built-in has no upstream catalog entry")
+		assert.Equal(t, directResolveBuiltin, decision, "describe_tool is a REAL direct-surface built-in")
+
+		// "retrieve_tools" has no "__" either, but it is a RETRIEVE-surface
+		// built-in, never registered on the direct surface at all — it must
+		// NOT be positively identified here. With a published, non-empty
+		// catalog that does not admit it, it is denied — never waved through
+		// on the strength of its shape alone.
+		entry, decision = p.resolveDirectTool("retrieve_tools")
+		assert.Nil(t, entry)
+		assert.Equal(t, directResolveDenied, decision,
+			"a name that is neither stamped in the catalog nor an explicit built-in has no registration identity")
 	})
 
 	t.Run("a withheld collision is denied in both id forms", func(t *testing.T) {
@@ -152,13 +163,21 @@ func publishPermsCatalog(p *MCPProxyServer, perms map[string]string) {
 // directToolPermissions map resolved this by accident — a nil map missed, and a
 // miss dropped the tool for a scoped agent — so the behaviour is preserved
 // deliberately here rather than left to be rediscovered.
+//
+// Spec 105 FR-008 (FR008-G2): the fixture's separator-less name is now
+// "describe_tool", a REAL direct-surface built-in identified from the
+// explicit set regardless of catalog state — not "retrieve_tools", which was
+// only ever kept here because a nil catalog's structural fallback could not
+// tell it apart from a genuine built-in (see the sibling test above). A
+// caller-supplied name with no registration identity at all is dropped in the
+// NoCatalog window exactly as an upstream-shaped one is.
 func TestFilterDirectModeToolsForAuth_NoCatalogPreservesPreChangeBehaviour(t *testing.T) {
 	proxy := &MCPProxyServer{}
 	require.Nil(t, proxy.loadDirectCatalog(), "precondition: no catalog published")
 
 	tools := []mcp.Tool{
 		{Name: FormatDirectToolName("github", "get_issue")},
-		{Name: "retrieve_tools"},
+		{Name: "describe_tool"},
 	}
 
 	t.Run("unauthenticated caller keeps everything", func(t *testing.T) {
@@ -178,8 +197,9 @@ func TestFilterDirectModeToolsForAuth_NoCatalogPreservesPreChangeBehaviour(t *te
 		for _, tl := range got {
 			names = append(names, tl.Name)
 		}
-		assert.Equal(t, []string{"retrieve_tools"}, names,
+		assert.Equal(t, []string{"describe_tool"}, names,
 			"with no catalog the tier is unknown, so an upstream tool fails closed for a scoped "+
-				"agent — but a built-in, which has no tier to begin with, must survive")
+				"agent — but a REAL built-in, positively identified by name regardless of catalog "+
+				"state, must survive")
 	})
 }