fix(057): address Codex review of PR #622 (deny-all bypass + FR-011 metadata)

Round-1 review fixes for the in-proxy profiles impl (MCP-1281/MCP-1278):

1. code_execution empty-effective-set bypass (MUST-FIX). A deny-all profile
   (servers: []) or a non-overlapping token∩profile yields an empty effective
   allow-list, which the JS runtime treated as "allow all" — leaking every
   server through code_execution. Add ExecutionOptions.RestrictToAllowed:
   when an active profile is in context, the allow-list is enforced even when
   empty (empty set = deny all). Set by the profile path in
   handleCodeExecution.

2. FR-011 metadata written to wrong shape (MUST-FIX). withProfileMeta smuggled
   the slug into the intent submap, so records landed as
   metadata.intent.profile and error paths dropped it entirely. Thread an
   explicit profile slug through EmitActivityToolCallCompleted →
   metadata["profile"] at top level, on success AND error paths.

3. Spec amendment (no code change): document per-request profile resolution
   (hot-reload takes effect on the next request) in spec.md + data-model.md,
   replacing the inaccurate "snapshot-until-reconnect" wording. Per-request
   re-resolution is kept as the safer behavior.

Tests: jsruntime deny-all/non-empty enforcement + backward-compat; Activity
service metadata["profile"] (success+error, not nested); code_execution
deny-all/non-overlapping rejection at the profile path; upgraded the FR-011
integration test to assert the persisted record's metadata["profile"].

Related #622

Author: Dumbris

internal/jsruntime/runtime.go

@@ -15,10 +15,17 @@ type ExecutionOptions struct {
 	Input          map[string]interface{} // Input data accessible as global `input` variable
 	TimeoutMs      int                    // Execution timeout in milliseconds
 	MaxToolCalls   int                    // Maximum number of call_tool() invocations (0 = unlimited)
-	AllowedServers []string               // Whitelist of allowed server names (empty = all allowed)
+	AllowedServers []string               // Whitelist of allowed server names (empty = all allowed, unless RestrictToAllowed)
 	ExecutionID    string                 // Unique execution ID for logging (auto-generated if empty)
 	Language       string                 // Source language: "javascript" (default) or "typescript"
 
+	// RestrictToAllowed enforces AllowedServers even when it is empty. Set by the
+	// Spec 057 profile path: an active profile with an empty effective server set
+	// (deny-all profile, or a non-overlapping token∩profile) must deny ALL
+	// call_tool() invocations rather than fall back to the "empty = allow all"
+	// default. Leave false for unrestricted code_execution and agent-token scopes.
+	RestrictToAllowed bool
+
 	// Auth enforcement (Spec 031)
 	AuthContext        *AuthInfo            // Auth context for permission enforcement (nil = no restrictions)
 	ToolAnnotationFunc ToolAnnotationLookup // Function to look up tool annotations for permission checking
@@ -73,16 +80,17 @@ type ToolCaller interface {
 
 // ExecutionContext tracks the state of a single JavaScript execution
 type ExecutionContext struct {
-	ExecutionID      string
-	StartTime        time.Time
-	EndTime          *time.Time
-	Status           string // "running", "success", "error", "timeout"
-	ToolCalls        []ToolCallRecord
-	ResultValue      interface{}
-	ErrorDetails     *JsError
-	toolCaller       ToolCaller
-	maxToolCalls     int
-	allowedServerMap map[string]bool
+	ExecutionID       string
+	StartTime         time.Time
+	EndTime           *time.Time
+	Status            string // "running", "success", "error", "timeout"
+	ToolCalls         []ToolCallRecord
+	ResultValue       interface{}
+	ErrorDetails      *JsError
+	toolCaller        ToolCaller
+	maxToolCalls      int
+	allowedServerMap  map[string]bool
+	restrictToAllowed bool // enforce allowedServerMap even when empty (Spec 057 deny-all profile)
 
 	// Auth enforcement (Spec 031)
 	authInfo           *AuthInfo
@@ -133,6 +141,7 @@ func Execute(ctx context.Context, caller ToolCaller, code string, opts Execution
 		toolCaller:         caller,
 		maxToolCalls:       opts.MaxToolCalls,
 		allowedServerMap:   make(map[string]bool),
+		restrictToAllowed:  opts.RestrictToAllowed,
 		authInfo:           opts.AuthContext,
 		toolAnnotationFunc: opts.ToolAnnotationFunc,
 		maxPermissionLevel: "",
@@ -300,8 +309,10 @@ func (ec *ExecutionContext) makeCallToolFunction(vm *goja.Runtime) func(goja.Fun
 			})
 		}
 
-		// Check allowed servers
-		if len(ec.allowedServerMap) > 0 && !ec.allowedServerMap[serverName] {
+		// Check allowed servers. When restrictToAllowed is set (active Spec 057
+		// profile), the map is enforced even when empty — an empty effective set
+		// means "deny everything". Otherwise an empty map means "no restriction".
+		if (ec.restrictToAllowed || len(ec.allowedServerMap) > 0) && !ec.allowedServerMap[serverName] {
 			return vm.ToValue(map[string]interface{}{
 				"ok": false,
 				"error": map[string]interface{}{

internal/jsruntime/runtime_test.go

@@ -308,6 +308,92 @@ func TestExecuteAllowedServers(t *testing.T) {
 	}
 }
 
+// TestExecuteRestrictToAllowedEmptyDeniesAll verifies that when RestrictToAllowed
+// is set — i.e. an active Spec 057 profile whose effective server set is empty
+// (deny-all profile, or a non-overlapping token∩profile intersection) — an empty
+// AllowedServers list denies ALL call_tool() invocations. Without this flag an
+// empty allow-list is treated as "allow all", which let the most locked-down
+// profiles leak every server through code_execution (Codex PR #622 finding #1).
+func TestExecuteRestrictToAllowedEmptyDeniesAll(t *testing.T) {
+	caller := newMockToolCaller()
+
+	code := `
+		var res = call_tool("anything", "tool", {});
+		({ ok: res.ok, code: res.error ? res.error.code : null })
+	`
+	opts := ExecutionOptions{
+		RestrictToAllowed: true,
+		AllowedServers:    nil, // active profile with empty effective set
+	}
+
+	result := Execute(context.Background(), caller, code, opts)
+	if !result.Ok {
+		t.Fatalf("expected ok=true (script itself ran), got error: %v", result.Error)
+	}
+
+	resultMap := result.Value.(map[string]interface{})
+	if resultMap["ok"] != false {
+		t.Errorf("expected call_tool ok=false (deny-all profile), got %v", resultMap["ok"])
+	}
+	if resultMap["code"] != string(ErrorCodeServerNotAllowed) {
+		t.Errorf("expected code=SERVER_NOT_ALLOWED, got %v", resultMap["code"])
+	}
+	if len(caller.calls) != 0 {
+		t.Errorf("expected upstream CallTool to never be reached, got %d calls", len(caller.calls))
+	}
+}
+
+// TestExecuteNoRestrictEmptyAllowsAll guards the backward-compatible path: with no
+// RestrictToAllowed flag and an empty AllowedServers list, call_tool() is allowed
+// (empty = no restriction). This must stay true for /mcp, /mcp/call, and
+// unrestricted code_execution after the finding-#1 fix.
+func TestExecuteNoRestrictEmptyAllowsAll(t *testing.T) {
+	caller := newMockToolCaller()
+
+	code := `var res = call_tool("anything", "tool", {}); ({ ok: res.ok })`
+	opts := ExecutionOptions{} // no restriction, empty allow-list
+
+	result := Execute(context.Background(), caller, code, opts)
+	if !result.Ok {
+		t.Fatalf("expected ok=true, got error: %v", result.Error)
+	}
+	resultMap := result.Value.(map[string]interface{})
+	if resultMap["ok"] != true {
+		t.Errorf("expected call_tool ok=true (no restriction), got %v", resultMap["ok"])
+	}
+}
+
+// TestExecuteRestrictToAllowedEnforcesNonEmpty verifies RestrictToAllowed with a
+// non-empty list still allows listed servers and blocks unlisted ones.
+func TestExecuteRestrictToAllowedEnforcesNonEmpty(t *testing.T) {
+	caller := newMockToolCaller()
+
+	code := `
+		var a = call_tool("allowed", "tool", {});
+		var b = call_tool("blocked", "tool", {});
+		({ a_ok: a.ok, b_ok: b.ok, b_code: b.error ? b.error.code : null })
+	`
+	opts := ExecutionOptions{
+		RestrictToAllowed: true,
+		AllowedServers:    []string{"allowed"},
+	}
+
+	result := Execute(context.Background(), caller, code, opts)
+	if !result.Ok {
+		t.Fatalf("expected ok=true, got error: %v", result.Error)
+	}
+	resultMap := result.Value.(map[string]interface{})
+	if resultMap["a_ok"] != true {
+		t.Errorf("expected a_ok=true (allowed server), got %v", resultMap["a_ok"])
+	}
+	if resultMap["b_ok"] != false {
+		t.Errorf("expected b_ok=false (blocked server), got %v", resultMap["b_ok"])
+	}
+	if resultMap["b_code"] != string(ErrorCodeServerNotAllowed) {
+		t.Errorf("expected b_code=SERVER_NOT_ALLOWED, got %v", resultMap["b_code"])
+	}
+}
+
 // TestExecuteNonSerializableResult tests rejection of non-JSON-serializable results
 func TestExecuteNonSerializableResult(t *testing.T) {
 	caller := newMockToolCaller()

internal/runtime/activity_service.go

@@ -277,6 +277,8 @@ func (s *ActivityService) handleToolCallCompleted(evt Event) {
 	intent := getMapPayload(evt.Payload, "intent")
 	// Extract content trust metadata if present (Spec 035)
 	contentTrust := getStringPayload(evt.Payload, "content_trust")
+	// Spec 057 FR-011: profile slug for tool calls from a /mcp/p/<slug> URL.
+	profileSlug := getStringPayload(evt.Payload, "profile")
 	// Default source to "mcp" if not specified (backwards compatibility)
 	activitySource := storage.ActivitySourceMCP
 	if source != "" {
@@ -285,7 +287,7 @@ func (s *ActivityService) handleToolCallCompleted(evt Event) {
 
 	// Build metadata with intent information if present
 	var metadata map[string]interface{}
-	if toolVariant != "" || intent != nil || contentTrust != "" {
+	if toolVariant != "" || intent != nil || contentTrust != "" || profileSlug != "" {
 		metadata = make(map[string]interface{})
 		if toolVariant != "" {
 			metadata["tool_variant"] = toolVariant
@@ -297,6 +299,11 @@ func (s *ActivityService) handleToolCallCompleted(evt Event) {
 		if contentTrust != "" {
 			metadata["content_trust"] = contentTrust
 		}
+		// Spec 057 FR-011: top-level profile slug (NOT nested under intent), so
+		// operators can correlate activity to the profile it came from.
+		if profileSlug != "" {
+			metadata["profile"] = profileSlug
+		}
 	}
 
 	// Spec 069 A1: byte sizes measured pre-truncation by the emitter.

internal/runtime/activity_service_test.go

@@ -565,6 +565,87 @@ func TestHandleToolCallCompleted_NoUserIdentity(t *testing.T) {
 	assert.Empty(t, record.UserEmail, "UserEmail should be empty when no _auth_user_email is present")
 }
 
+// TestHandleToolCallCompleted_ProfileMetadata verifies Spec 057 FR-011: the
+// profile slug from a /mcp/p/<slug> tool call lands at the TOP-LEVEL
+// metadata["profile"], NOT smuggled under metadata.intent.profile. Covers both
+// success and error paths (Codex PR #622 finding #2).
+func TestHandleToolCallCompleted_ProfileMetadata(t *testing.T) {
+	for _, status := range []string{"success", "error"} {
+		t.Run(status, func(t *testing.T) {
+			store, cleanup := setupTestStorage(t)
+			defer cleanup()
+
+			svc := NewActivityService(store, zap.NewNop())
+
+			evt := Event{
+				Type:      EventTypeActivityToolCallCompleted,
+				Timestamp: time.Now().UTC(),
+				Payload: map[string]any{
+					"server_name":  "research-srv",
+					"tool_name":    "search_papers",
+					"status":       status,
+					"duration_ms":  int64(10),
+					"tool_variant": "read",
+					"profile":      "research",
+					// An intent map is also present; profile must NOT live inside it.
+					"intent": map[string]interface{}{
+						"operation_type": "read",
+					},
+				},
+			}
+
+			svc.handleEvent(evt)
+
+			records, _, err := store.ListActivities(storage.DefaultActivityFilter())
+			require.NoError(t, err)
+			require.Len(t, records, 1)
+
+			md := records[0].Metadata
+			require.NotNil(t, md)
+			assert.Equal(t, "research", md["profile"],
+				"FR-011: profile slug must be at top-level metadata[\"profile\"]")
+
+			// Must NOT be nested inside the intent submap.
+			if intent, ok := md["intent"].(map[string]interface{}); ok {
+				_, nested := intent["profile"]
+				assert.False(t, nested, "profile must not be nested under metadata.intent.profile")
+			}
+		})
+	}
+}
+
+// TestHandleToolCallCompleted_NoProfileMetadata verifies that a tool call from
+// /mcp (no profile) omits metadata["profile"] entirely (FR-011 backward compat).
+func TestHandleToolCallCompleted_NoProfileMetadata(t *testing.T) {
+	store, cleanup := setupTestStorage(t)
+	defer cleanup()
+
+	svc := NewActivityService(store, zap.NewNop())
+
+	evt := Event{
+		Type:      EventTypeActivityToolCallCompleted,
+		Timestamp: time.Now().UTC(),
+		Payload: map[string]any{
+			"server_name":  "github",
+			"tool_name":    "list_repos",
+			"status":       "success",
+			"duration_ms":  int64(10),
+			"tool_variant": "read",
+		},
+	}
+
+	svc.handleEvent(evt)
+
+	records, _, err := store.ListActivities(storage.DefaultActivityFilter())
+	require.NoError(t, err)
+	require.Len(t, records, 1)
+
+	if md := records[0].Metadata; md != nil {
+		_, ok := md["profile"]
+		assert.False(t, ok, "records from /mcp must omit metadata[\"profile\"]")
+	}
+}
+
 // TestHandleToolCallCompleted_NilArguments verifies no panic when arguments is nil.
 func TestHandleToolCallCompleted_NilArguments(t *testing.T) {
 	store, cleanup := setupTestStorage(t)

internal/runtime/event_bus.go

@@ -397,7 +397,7 @@ func (r *Runtime) EmitActivityToolCallStarted(serverName, toolName, sessionID, r
 // arguments is the input parameters passed to the tool call
 // toolVariant is the MCP tool variant used (call_tool_read/write/destructive) - optional
 // intent is the intent declaration metadata - optional
-func (r *Runtime) EmitActivityToolCallCompleted(serverName, toolName, sessionID, requestID, source, status, errorMsg string, durationMs int64, arguments map[string]interface{}, response string, responseTruncated bool, toolVariant string, intent map[string]interface{}, contentTrust string, requestBytes, responseBytes int) {
+func (r *Runtime) EmitActivityToolCallCompleted(serverName, toolName, sessionID, requestID, source, status, errorMsg string, durationMs int64, arguments map[string]interface{}, response string, responseTruncated bool, toolVariant string, intent map[string]interface{}, contentTrust, profile string, requestBytes, responseBytes int) {
 	// Spec 042: classify failed tool calls into the upstream error categories.
 	// We never record the error message itself; only a fixed enum value.
 	if status == "error" && errorMsg != "" {
@@ -438,6 +438,10 @@ func (r *Runtime) EmitActivityToolCallCompleted(serverName, toolName, sessionID,
 	if contentTrust != "" {
 		payload["content_trust"] = contentTrust
 	}
+	// Spec 057 FR-011: profile slug for tool calls from a /mcp/p/<slug> URL.
+	if profile != "" {
+		payload["profile"] = profile
+	}
 	// Spec 069 A1: pre-truncation byte sizes (0 means not measured / legacy callers)
 	if requestBytes > 0 {
 		payload["request_bytes"] = requestBytes