Skip to content

Commit b11fcb2

Browse files
SamMorrowDrumspatrick-knightCopilot
committed
fix(governance): validate custom property values
Co-authored-by: Patrick Knight <patrick-knight@github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e886867-a922-419a-b02c-ac643716aea8
1 parent 78c6b37 commit b11fcb2

3 files changed

Lines changed: 175 additions & 33 deletions

File tree

‎pkg/github/__toolsnaps__/custom_properties_write.snap‎

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,18 @@
4040
"type": "array"
4141
},
4242
"default_value": {
43-
"description": "Organization and enterprise levels only: the value applied when a repository does not set the property. A string or an array of strings."
43+
"description": "Organization and enterprise levels only: the value applied when a repository does not set the property. A string or an array of strings.",
44+
"oneOf": [
45+
{
46+
"type": "string"
47+
},
48+
{
49+
"items": {
50+
"type": "string"
51+
},
52+
"type": "array"
53+
}
54+
]
4455
},
4556
"description": {
4657
"description": "Organization and enterprise levels only: a short description of the property.",
@@ -55,7 +66,21 @@
5566
"type": "boolean"
5667
},
5768
"value": {
58-
"description": "Repository level only: the value to assign. A string, an array of strings, or null to clear the value."
69+
"description": "Repository level only: the value to assign. A string, an array of strings, or null to clear the value.",
70+
"oneOf": [
71+
{
72+
"type": "string"
73+
},
74+
{
75+
"items": {
76+
"type": "string"
77+
},
78+
"type": "array"
79+
},
80+
{
81+
"type": "null"
82+
}
83+
]
5984
},
6085
"value_type": {
6186
"description": "Organization and enterprise levels only: the data type of the property. Required when defining a property.",

‎pkg/github/custom_properties.go‎

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -14,19 +14,12 @@ import (
1414
"github.com/modelcontextprotocol/go-sdk/mcp"
1515
)
1616

17-
// customPropertiesLevelDescription documents the "level" parameter shared by
18-
// the custom properties read and write tools. The repository level operates on
19-
// the property VALUES assigned to a repository, while the organization and
20-
// enterprise levels operate on the property DEFINITIONS (schema).
2117
const customPropertiesLevelDescription = "The level at which custom properties are managed:\n" +
2218
"- 'repository': The custom property VALUES assigned to a repository (requires 'owner' and 'repo').\n" +
2319
"- 'organization': The custom property DEFINITIONS (schema) for an organization (requires 'org').\n" +
2420
"- 'enterprise': The custom property DEFINITIONS (schema) for an enterprise (requires 'enterprise')."
2521

26-
// CustomPropertiesRead creates a tool for read operations on custom properties
27-
// at the repository, organization, or enterprise level. The level is selected
28-
// with the "level" parameter. Repository reads return property values;
29-
// organization and enterprise reads return property definitions.
22+
// CustomPropertiesRead creates the custom properties read tool.
3023
func CustomPropertiesRead(t translations.TranslationHelperFunc) inventory.ServerTool {
3124
return NewTool(
3225
ToolsetMetadataGovernance,
@@ -91,7 +84,6 @@ func CustomPropertiesRead(t translations.TranslationHelperFunc) inventory.Server
9184
)
9285
}
9386

94-
// customPropertiesReadRepository handles custom_properties_read calls with level="repository".
9587
func customPropertiesReadRepository(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
9688
owner, err := RequiredParam[string](args, "owner")
9789
if err != nil {
@@ -113,7 +105,6 @@ func customPropertiesReadRepository(ctx context.Context, client *github.Client,
113105
return MarshalledTextResult(properties), nil, nil
114106
}
115107

116-
// customPropertiesReadOrganization handles custom_properties_read calls with level="organization".
117108
func customPropertiesReadOrganization(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
118109
org, err := RequiredParam[string](args, "org")
119110
if err != nil {
@@ -131,7 +122,6 @@ func customPropertiesReadOrganization(ctx context.Context, client *github.Client
131122
return MarshalledTextResult(properties), nil, nil
132123
}
133124

134-
// customPropertiesReadEnterprise handles custom_properties_read calls with level="enterprise".
135125
func customPropertiesReadEnterprise(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
136126
enterprise, err := RequiredParam[string](args, "enterprise")
137127
if err != nil {
@@ -149,10 +139,7 @@ func customPropertiesReadEnterprise(ctx context.Context, client *github.Client,
149139
return MarshalledTextResult(properties), nil, nil
150140
}
151141

152-
// CustomPropertiesWrite creates a tool for create-or-update operations on
153-
// custom properties at the repository, organization, or enterprise level. The
154-
// level is selected with the "level" parameter. Repository writes set property
155-
// values; organization and enterprise writes define property schemas.
142+
// CustomPropertiesWrite creates the custom properties write tool.
156143
func CustomPropertiesWrite(t translations.TranslationHelperFunc) inventory.ServerTool {
157144
return NewTool(
158145
ToolsetMetadataGovernance,
@@ -222,7 +209,6 @@ func CustomPropertiesWrite(t translations.TranslationHelperFunc) inventory.Serve
222209
)
223210
}
224211

225-
// customPropertiesWriteRepository handles custom_properties_write calls with level="repository".
226212
func customPropertiesWriteRepository(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
227213
owner, err := RequiredParam[string](args, "owner")
228214
if err != nil {
@@ -232,7 +218,7 @@ func customPropertiesWriteRepository(ctx context.Context, client *github.Client,
232218
if err != nil {
233219
return utils.NewToolResultError(err.Error()), nil, nil
234220
}
235-
values, errResult := parseCustomProperties[*github.CustomPropertyValue](args)
221+
values, errResult := parseCustomProperties[*github.CustomPropertyValue](args, "value")
236222
if errResult != nil {
237223
return errResult, nil, nil
238224
}
@@ -248,13 +234,12 @@ func customPropertiesWriteRepository(ctx context.Context, client *github.Client,
248234
return utils.NewToolResultText("Repository custom property values updated successfully"), nil, nil
249235
}
250236

251-
// customPropertiesWriteOrganization handles custom_properties_write calls with level="organization".
252237
func customPropertiesWriteOrganization(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
253238
org, err := RequiredParam[string](args, "org")
254239
if err != nil {
255240
return utils.NewToolResultError(err.Error()), nil, nil
256241
}
257-
properties, errResult := parseCustomProperties[*github.CustomProperty](args)
242+
properties, errResult := parseCustomProperties[*github.CustomProperty](args, "value_type")
258243
if errResult != nil {
259244
return errResult, nil, nil
260245
}
@@ -270,13 +255,12 @@ func customPropertiesWriteOrganization(ctx context.Context, client *github.Clien
270255
return MarshalledTextResult(updated), nil, nil
271256
}
272257

273-
// customPropertiesWriteEnterprise handles custom_properties_write calls with level="enterprise".
274258
func customPropertiesWriteEnterprise(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
275259
enterprise, err := RequiredParam[string](args, "enterprise")
276260
if err != nil {
277261
return utils.NewToolResultError(err.Error()), nil, nil
278262
}
279-
properties, errResult := parseCustomProperties[*github.CustomProperty](args)
263+
properties, errResult := parseCustomProperties[*github.CustomProperty](args, "value_type")
280264
if errResult != nil {
281265
return errResult, nil, nil
282266
}
@@ -292,10 +276,7 @@ func customPropertiesWriteEnterprise(ctx context.Context, client *github.Client,
292276
return MarshalledTextResult(updated), nil, nil
293277
}
294278

295-
// customPropertyItemSchema describes a single item of the "properties" array
296-
// for custom_properties_write. It covers both a repository property value and
297-
// an organization or enterprise property definition; which fields apply
298-
// depends on the "level" argument.
279+
// customPropertyItemSchema combines repository values with organization and enterprise definitions.
299280
func customPropertyItemSchema() *jsonschema.Schema {
300281
return &jsonschema.Schema{
301282
Type: "object",
@@ -306,6 +287,14 @@ func customPropertyItemSchema() *jsonschema.Schema {
306287
},
307288
"value": {
308289
Description: "Repository level only: the value to assign. A string, an array of strings, or null to clear the value.",
290+
OneOf: []*jsonschema.Schema{
291+
{Type: "string"},
292+
{
293+
Type: "array",
294+
Items: &jsonschema.Schema{Type: "string"},
295+
},
296+
{Type: "null"},
297+
},
309298
},
310299
"value_type": {
311300
Type: "string",
@@ -318,6 +307,13 @@ func customPropertyItemSchema() *jsonschema.Schema {
318307
},
319308
"default_value": {
320309
Description: "Organization and enterprise levels only: the value applied when a repository does not set the property. A string or an array of strings.",
310+
OneOf: []*jsonschema.Schema{
311+
{Type: "string"},
312+
{
313+
Type: "array",
314+
Items: &jsonschema.Schema{Type: "string"},
315+
},
316+
},
321317
},
322318
"description": {
323319
Type: "string",
@@ -338,10 +334,7 @@ func customPropertyItemSchema() *jsonschema.Schema {
338334
}
339335
}
340336

341-
// parseCustomProperties reads the "properties" array argument and decodes it into
342-
// the requested go-github type. It returns a non-nil *mcp.CallToolResult
343-
// describing the problem when the argument is missing or malformed.
344-
func parseCustomProperties[T any](args map[string]any) ([]T, *mcp.CallToolResult) {
337+
func parseCustomProperties[T any](args map[string]any, requiredItemField string) ([]T, *mcp.CallToolResult) {
345338
raw, ok := args["properties"]
346339
if !ok || raw == nil {
347340
return nil, utils.NewToolResultError("properties parameter is required")
@@ -350,6 +343,15 @@ func parseCustomProperties[T any](args map[string]any) ([]T, *mcp.CallToolResult
350343
if !ok {
351344
return nil, utils.NewToolResultError("properties parameter must be an array")
352345
}
346+
for i, rawProperty := range arr {
347+
property, ok := rawProperty.(map[string]any)
348+
if !ok {
349+
return nil, utils.NewToolResultError(fmt.Sprintf("properties[%d] must be an object", i))
350+
}
351+
if _, ok := property[requiredItemField]; !ok {
352+
return nil, utils.NewToolResultError(fmt.Sprintf("properties[%d].%s is required", i, requiredItemField))
353+
}
354+
}
353355

354356
encoded, err := json.Marshal(arr)
355357
if err != nil {

‎pkg/github/custom_properties_test.go‎

Lines changed: 117 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,65 @@ func Test_CustomPropertiesWrite(t *testing.T) {
134134
require.True(t, ok, "InputSchema should be *jsonschema.Schema")
135135
assert.ElementsMatch(t, schema.Required, []string{"level", "properties"})
136136

137+
t.Run("value schemas enforce documented JSON types", func(t *testing.T) {
138+
defaultValueSchema := schema.Properties["properties"].Items.Properties["default_value"]
139+
require.Len(t, defaultValueSchema.OneOf, 2)
140+
assert.Equal(t, "string", defaultValueSchema.OneOf[0].Type)
141+
assert.Equal(t, "array", defaultValueSchema.OneOf[1].Type)
142+
assert.Equal(t, "string", defaultValueSchema.OneOf[1].Items.Type)
143+
144+
valueSchema := schema.Properties["properties"].Items.Properties["value"]
145+
require.Len(t, valueSchema.OneOf, 3)
146+
assert.Equal(t, "string", valueSchema.OneOf[0].Type)
147+
assert.Equal(t, "array", valueSchema.OneOf[1].Type)
148+
assert.Equal(t, "string", valueSchema.OneOf[1].Items.Type)
149+
assert.Equal(t, "null", valueSchema.OneOf[2].Type)
150+
151+
resolved, err := schema.Resolve(nil)
152+
require.NoError(t, err)
153+
154+
tests := []struct {
155+
name string
156+
field string
157+
value any
158+
shouldPass bool
159+
}{
160+
{name: "default string", field: "default_value", value: "production", shouldPass: true},
161+
{name: "default string array", field: "default_value", value: []any{"production", "staging"}, shouldPass: true},
162+
{name: "default number", field: "default_value", value: 1},
163+
{name: "default boolean", field: "default_value", value: true},
164+
{name: "default object", field: "default_value", value: map[string]any{"environment": "production"}},
165+
{name: "default null", field: "default_value", value: nil},
166+
{name: "value string", field: "value", value: "production", shouldPass: true},
167+
{name: "value string array", field: "value", value: []any{"production", "staging"}, shouldPass: true},
168+
{name: "value null", field: "value", value: nil, shouldPass: true},
169+
{name: "value number", field: "value", value: 1},
170+
{name: "value boolean", field: "value", value: true},
171+
{name: "value object", field: "value", value: map[string]any{"environment": "production"}},
172+
}
173+
174+
for _, tt := range tests {
175+
t.Run(tt.name, func(t *testing.T) {
176+
level := "repository"
177+
property := map[string]any{"property_name": "environment"}
178+
if tt.field == "default_value" {
179+
level = "organization"
180+
property["value_type"] = "single_select"
181+
}
182+
property[tt.field] = tt.value
183+
err := resolved.Validate(map[string]any{
184+
"level": level,
185+
"properties": []any{property},
186+
})
187+
if tt.shouldPass {
188+
require.NoError(t, err)
189+
} else {
190+
require.Error(t, err)
191+
}
192+
})
193+
}
194+
})
195+
137196
t.Run("repository level: sets property values", func(t *testing.T) {
138197
var captured struct {
139198
Properties []*github.CustomPropertyValue `json:"properties"`
@@ -153,6 +212,7 @@ func Test_CustomPropertiesWrite(t *testing.T) {
153212
"repo": "repo",
154213
"properties": []any{
155214
map[string]any{"property_name": "environment", "value": "production"},
215+
map[string]any{"property_name": "deprecated", "value": nil},
156216
},
157217
})
158218

@@ -161,9 +221,11 @@ func Test_CustomPropertiesWrite(t *testing.T) {
161221
require.False(t, result.IsError)
162222
assert.Contains(t, getTextResult(t, result).Text, "updated successfully")
163223

164-
require.Len(t, captured.Properties, 1)
224+
require.Len(t, captured.Properties, 2)
165225
assert.Equal(t, "environment", captured.Properties[0].PropertyName)
166226
assert.Equal(t, "production", captured.Properties[0].Value)
227+
assert.Equal(t, "deprecated", captured.Properties[1].PropertyName)
228+
assert.Nil(t, captured.Properties[1].Value)
167229
})
168230

169231
t.Run("repository level: requires properties", func(t *testing.T) {
@@ -178,6 +240,48 @@ func Test_CustomPropertiesWrite(t *testing.T) {
178240
assert.Contains(t, getErrorResult(t, result).Text, "properties parameter is required")
179241
})
180242

243+
t.Run("requires level-specific property fields", func(t *testing.T) {
244+
tests := []struct {
245+
name string
246+
args map[string]any
247+
requiredPath string
248+
}{
249+
{
250+
name: "repository value",
251+
args: map[string]any{
252+
"level": "repository",
253+
"owner": "owner",
254+
"repo": "repo",
255+
"properties": []any{map[string]any{"property_name": "environment"}},
256+
},
257+
requiredPath: "properties[0].value",
258+
},
259+
{
260+
name: "organization value_type",
261+
args: map[string]any{
262+
"level": "organization",
263+
"org": "octo",
264+
"properties": []any{map[string]any{"property_name": "environment"}},
265+
},
266+
requiredPath: "properties[0].value_type",
267+
},
268+
}
269+
270+
for _, tt := range tests {
271+
t.Run(tt.name, func(t *testing.T) {
272+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{}))
273+
deps := BaseDeps{Client: client}
274+
handler := toolDef.Handler(deps)
275+
request := createMCPRequest(tt.args)
276+
277+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
278+
require.NoError(t, err)
279+
require.True(t, result.IsError)
280+
assert.Contains(t, getErrorResult(t, result).Text, tt.requiredPath)
281+
})
282+
}
283+
})
284+
181285
t.Run("organization level: defines property schema", func(t *testing.T) {
182286
var captured struct {
183287
Properties []*github.CustomProperty `json:"properties"`
@@ -200,6 +304,7 @@ func Test_CustomPropertiesWrite(t *testing.T) {
200304
"property_name": "environment",
201305
"value_type": "single_select",
202306
"required": true,
307+
"default_value": "production",
203308
"allowed_values": []any{"production", "staging"},
204309
},
205310
},
@@ -213,6 +318,9 @@ func Test_CustomPropertiesWrite(t *testing.T) {
213318
assert.Equal(t, "environment", captured.Properties[0].GetPropertyName())
214319
assert.Equal(t, github.PropertyValueType("single_select"), captured.Properties[0].ValueType)
215320
assert.ElementsMatch(t, []string{"production", "staging"}, captured.Properties[0].AllowedValues)
321+
defaultValue, ok := captured.Properties[0].DefaultValueString()
322+
require.True(t, ok)
323+
assert.Equal(t, "production", defaultValue)
216324
})
217325

218326
t.Run("enterprise level: defines property schema", func(t *testing.T) {
@@ -233,7 +341,11 @@ func Test_CustomPropertiesWrite(t *testing.T) {
233341
"level": "enterprise",
234342
"enterprise": "acme",
235343
"properties": []any{
236-
map[string]any{"property_name": "compliance", "value_type": "true_false"},
344+
map[string]any{
345+
"property_name": "compliance",
346+
"value_type": "multi_select",
347+
"default_value": []any{"soc2", "fedramp"},
348+
},
237349
},
238350
})
239351

@@ -243,6 +355,9 @@ func Test_CustomPropertiesWrite(t *testing.T) {
243355

244356
require.Len(t, captured.Properties, 1)
245357
assert.Equal(t, "compliance", captured.Properties[0].GetPropertyName())
358+
defaultValues, ok := captured.Properties[0].DefaultValueStrings()
359+
require.True(t, ok)
360+
assert.Equal(t, []string{"soc2", "fedramp"}, defaultValues)
246361
})
247362

248363
t.Run("unknown level returns an error", func(t *testing.T) {

0 commit comments

Comments
 (0)