Skip to content

Commit 3463a42

Browse files
SamMorrowDrumspatrick-knightCopilot
committed
fix(governance): preserve partial property updates
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 8e921ba commit 3463a42

3 files changed

Lines changed: 426 additions & 182 deletions

File tree

‎pkg/github/__toolsnaps__/custom_properties_write.snap‎

Lines changed: 45 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44
"readOnlyHint": false,
55
"title": "Set custom properties"
66
},
7-
"description": "Create or update custom properties at the repository, organization, or enterprise level. At the repository level this sets the property values on a repository (the properties must already be defined for the organization); at the organization and enterprise levels it creates or updates the property definitions (schema). Select the level with the 'level' parameter.",
7+
"description": "Create or update custom properties at the repository, organization, or enterprise level. At the repository level this sets the property values on a repository (the properties must already be defined for the organization). Organization and enterprise definition writes preserve omitted writable fields by reading current definitions immediately before updating; concurrent definition updates remain last-write-wins. Select the level with the 'level' parameter.",
88
"inputSchema": {
99
"properties": {
1010
"enterprise": {
@@ -69,14 +69,21 @@
6969
"description": "An organization- or enterprise-level custom property definition.",
7070
"properties": {
7171
"allowed_values": {
72-
"description": "The ordered list of allowed values for single_select and multi_select properties.",
73-
"items": {
74-
"type": "string"
75-
},
76-
"type": "array"
72+
"anyOf": [
73+
{
74+
"items": {
75+
"type": "string"
76+
},
77+
"type": "array"
78+
},
79+
{
80+
"type": "null"
81+
}
82+
],
83+
"description": "The ordered list of allowed values for single_select and multi_select properties. Omit when updating to preserve the current list; use null or an empty array to clear it."
7784
},
7885
"default_value": {
79-
"description": "The value applied when a repository does not set the property. A string or an array of strings.",
86+
"description": "The value applied when a repository does not set the property. Omit when updating to preserve the current value; use null to clear it.",
8087
"oneOf": [
8188
{
8289
"type": "string"
@@ -86,23 +93,37 @@
8693
"type": "string"
8794
},
8895
"type": "array"
96+
},
97+
{
98+
"type": "null"
8999
}
90100
]
91101
},
92102
"description": {
93-
"description": "A short description of the property.",
94-
"type": "string"
103+
"anyOf": [
104+
{
105+
"type": "string"
106+
},
107+
{
108+
"type": "null"
109+
}
110+
],
111+
"description": "A short description of the property. Omit when updating to preserve the current description; use null to clear it."
95112
},
96113
"property_name": {
97114
"description": "The name of the custom property.",
98115
"type": "string"
99116
},
117+
"require_explicit_values": {
118+
"description": "Whether repositories must explicitly set a value for the property. Omit when updating to preserve the current setting.",
119+
"type": "boolean"
120+
},
100121
"required": {
101-
"description": "Whether the property must be set on every repository.",
122+
"description": "Whether the property must be set on every repository. Omit when updating to preserve the current setting.",
102123
"type": "boolean"
103124
},
104125
"value_type": {
105-
"description": "The data type of the property.",
126+
"description": "The data type of the property. Required for new definitions; omit when updating to preserve the current type.",
106127
"enum": [
107128
"string",
108129
"single_select",
@@ -113,17 +134,23 @@
113134
"type": "string"
114135
},
115136
"values_editable_by": {
116-
"description": "Who can edit the values of the property.",
117-
"enum": [
118-
"org_actors",
119-
"org_and_repo_actors"
137+
"anyOf": [
138+
{
139+
"enum": [
140+
"org_actors",
141+
"org_and_repo_actors"
142+
],
143+
"type": "string"
144+
},
145+
{
146+
"type": "null"
147+
}
120148
],
121-
"type": "string"
149+
"description": "Who can edit the values of the property. Omit when updating to preserve the current setting; use null to restore the default."
122150
}
123151
},
124152
"required": [
125-
"property_name",
126-
"value_type"
153+
"property_name"
127154
],
128155
"type": "object"
129156
}

‎pkg/github/custom_properties.go‎

Lines changed: 150 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
"context"
55
"encoding/json"
66
"fmt"
7+
"net/http"
8+
"net/url"
79

810
ghErrors "github.com/github/github-mcp-server/pkg/errors"
911
"github.com/github/github-mcp-server/pkg/inventory"
@@ -111,12 +113,9 @@ func customPropertiesReadOrganization(ctx context.Context, client *github.Client
111113
return utils.NewToolResultError(err.Error()), nil, nil
112114
}
113115

114-
properties, resp, err := client.Organizations.GetAllCustomProperties(ctx, org)
115-
if resp != nil {
116-
defer func() { _ = resp.Body.Close() }()
117-
}
118-
if err != nil {
119-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get organization custom properties", resp, err), nil, nil
116+
properties, errResult := getCustomPropertyDefinitions(ctx, client, fmt.Sprintf("orgs/%s/properties/schema", url.PathEscape(org)), "failed to get organization custom properties")
117+
if errResult != nil {
118+
return errResult, nil, nil
120119
}
121120

122121
return MarshalledTextResult(properties), nil, nil
@@ -128,12 +127,9 @@ func customPropertiesReadEnterprise(ctx context.Context, client *github.Client,
128127
return utils.NewToolResultError(err.Error()), nil, nil
129128
}
130129

131-
properties, resp, err := client.Enterprise.GetAllCustomProperties(ctx, enterprise)
132-
if resp != nil {
133-
defer func() { _ = resp.Body.Close() }()
134-
}
135-
if err != nil {
136-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get enterprise custom properties", resp, err), nil, nil
130+
properties, errResult := getCustomPropertyDefinitions(ctx, client, fmt.Sprintf("enterprises/%s/properties/schema", url.PathEscape(enterprise)), "failed to get enterprise custom properties")
131+
if errResult != nil {
132+
return errResult, nil, nil
137133
}
138134

139135
return MarshalledTextResult(properties), nil, nil
@@ -145,7 +141,7 @@ func CustomPropertiesWrite(t translations.TranslationHelperFunc) inventory.Serve
145141
ToolsetMetadataGovernance,
146142
mcp.Tool{
147143
Name: "custom_properties_write",
148-
Description: t("TOOL_CUSTOM_PROPERTIES_WRITE_DESCRIPTION", "Create or update custom properties at the repository, organization, or enterprise level. At the repository level this sets the property values on a repository (the properties must already be defined for the organization); at the organization and enterprise levels it creates or updates the property definitions (schema). Select the level with the 'level' parameter."),
144+
Description: t("TOOL_CUSTOM_PROPERTIES_WRITE_DESCRIPTION", "Create or update custom properties at the repository, organization, or enterprise level. At the repository level this sets the property values on a repository (the properties must already be defined for the organization). Organization and enterprise definition writes preserve omitted writable fields by reading current definitions immediately before updating; concurrent definition updates remain last-write-wins. Select the level with the 'level' parameter."),
149145
Annotations: &mcp.ToolAnnotations{
150146
Title: t("TOOL_CUSTOM_PROPERTIES_WRITE_USER_TITLE", "Set custom properties"),
151147
ReadOnlyHint: false,
@@ -245,41 +241,25 @@ func customPropertiesWriteOrganization(ctx context.Context, client *github.Clien
245241
if err != nil {
246242
return utils.NewToolResultError(err.Error()), nil, nil
247243
}
248-
properties, errResult := parseCustomProperties[*github.CustomProperty](args, customPropertyDefinitionSchema())
244+
properties, errResult := parseCustomProperties[map[string]any](args, customPropertyDefinitionSchema())
249245
if errResult != nil {
250246
return errResult, nil, nil
251247
}
252248

253-
updated, resp, err := client.Organizations.CreateOrUpdateCustomProperties(ctx, org, properties)
254-
if resp != nil {
255-
defer func() { _ = resp.Body.Close() }()
256-
}
257-
if err != nil {
258-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update organization custom properties", resp, err), nil, nil
259-
}
260-
261-
return MarshalledTextResult(updated), nil, nil
249+
return customPropertiesWriteDefinitions(ctx, client, fmt.Sprintf("orgs/%s/properties/schema", url.PathEscape(org)), "organization", properties)
262250
}
263251

264252
func customPropertiesWriteEnterprise(ctx context.Context, client *github.Client, args map[string]any) (*mcp.CallToolResult, any, error) {
265253
enterprise, err := RequiredParam[string](args, "enterprise")
266254
if err != nil {
267255
return utils.NewToolResultError(err.Error()), nil, nil
268256
}
269-
properties, errResult := parseCustomProperties[*github.CustomProperty](args, customPropertyDefinitionSchema())
257+
properties, errResult := parseCustomProperties[map[string]any](args, customPropertyDefinitionSchema())
270258
if errResult != nil {
271259
return errResult, nil, nil
272260
}
273261

274-
updated, resp, err := client.Enterprise.CreateOrUpdateCustomProperties(ctx, enterprise, properties)
275-
if resp != nil {
276-
defer func() { _ = resp.Body.Close() }()
277-
}
278-
if err != nil {
279-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to update enterprise custom properties", resp, err), nil, nil
280-
}
281-
282-
return MarshalledTextResult(updated), nil, nil
262+
return customPropertiesWriteDefinitions(ctx, client, fmt.Sprintf("enterprises/%s/properties/schema", url.PathEscape(enterprise)), "enterprise", properties)
283263
}
284264

285265
func customPropertyValueSchema() *jsonschema.Schema {
@@ -321,39 +301,164 @@ func customPropertyDefinitionSchema() *jsonschema.Schema {
321301
"value_type": {
322302
Type: "string",
323303
Enum: []any{"string", "single_select", "multi_select", "true_false", "url"},
324-
Description: "The data type of the property.",
304+
Description: "The data type of the property. Required for new definitions; omit when updating to preserve the current type.",
325305
},
326306
"required": {
327307
Type: "boolean",
328-
Description: "Whether the property must be set on every repository.",
308+
Description: "Whether the property must be set on every repository. Omit when updating to preserve the current setting.",
329309
},
330310
"default_value": {
331-
Description: "The value applied when a repository does not set the property. A string or an array of strings.",
311+
Description: "The value applied when a repository does not set the property. Omit when updating to preserve the current value; use null to clear it.",
332312
OneOf: []*jsonschema.Schema{
333313
{Type: "string"},
334314
{
335315
Type: "array",
336316
Items: &jsonschema.Schema{Type: "string"},
337317
},
318+
{Type: "null"},
338319
},
339320
},
340321
"description": {
341-
Type: "string",
342-
Description: "A short description of the property.",
322+
Description: "A short description of the property. Omit when updating to preserve the current description; use null to clear it.",
323+
AnyOf: []*jsonschema.Schema{
324+
{Type: "string"},
325+
{Type: "null"},
326+
},
343327
},
344328
"allowed_values": {
345-
Type: "array",
346-
Description: "The ordered list of allowed values for single_select and multi_select properties.",
347-
Items: &jsonschema.Schema{Type: "string"},
329+
Description: "The ordered list of allowed values for single_select and multi_select properties. Omit when updating to preserve the current list; use null or an empty array to clear it.",
330+
AnyOf: []*jsonschema.Schema{
331+
{
332+
Type: "array",
333+
Items: &jsonschema.Schema{Type: "string"},
334+
},
335+
{Type: "null"},
336+
},
348337
},
349338
"values_editable_by": {
350-
Type: "string",
351-
Enum: []any{"org_actors", "org_and_repo_actors"},
352-
Description: "Who can edit the values of the property.",
339+
Description: "Who can edit the values of the property. Omit when updating to preserve the current setting; use null to restore the default.",
340+
AnyOf: []*jsonschema.Schema{
341+
{
342+
Type: "string",
343+
Enum: []any{"org_actors", "org_and_repo_actors"},
344+
},
345+
{Type: "null"},
346+
},
347+
},
348+
"require_explicit_values": {
349+
Type: "boolean",
350+
Description: "Whether repositories must explicitly set a value for the property. Omit when updating to preserve the current setting.",
353351
},
354352
},
355-
Required: []string{"property_name", "value_type"},
353+
Required: []string{"property_name"},
354+
}
355+
}
356+
357+
var customPropertyDefinitionFields = []string{
358+
"value_type",
359+
"required",
360+
"default_value",
361+
"description",
362+
"allowed_values",
363+
"values_editable_by",
364+
"require_explicit_values",
365+
}
366+
367+
func customPropertiesWriteDefinitions(ctx context.Context, client *github.Client, apiURL, sourceType string, requested []map[string]any) (*mcp.CallToolResult, any, error) {
368+
if errResult := validateUniqueCustomPropertyNames(requested); errResult != nil {
369+
return errResult, nil, nil
370+
}
371+
372+
current, errResult := getCustomPropertyDefinitions(ctx, client, apiURL, fmt.Sprintf("failed to get %s custom properties before updating", sourceType))
373+
if errResult != nil {
374+
return errResult, nil, nil
375+
}
376+
merged, errResult := mergeCustomPropertyDefinitions(current, requested, sourceType)
377+
if errResult != nil {
378+
return errResult, nil, nil
379+
}
380+
381+
req, err := client.NewRequest(ctx, http.MethodPatch, apiURL, map[string]any{"properties": merged})
382+
if err != nil {
383+
return utils.NewToolResultErrorFromErr("failed to create custom properties update request", err), nil, nil
384+
}
385+
386+
var updated []map[string]any
387+
resp, err := client.Do(req, &updated)
388+
if resp != nil {
389+
defer func() { _ = resp.Body.Close() }()
390+
}
391+
if err != nil {
392+
return ghErrors.NewGitHubAPIErrorResponse(ctx, fmt.Sprintf("failed to update %s custom properties", sourceType), resp, err), nil, nil
393+
}
394+
return MarshalledTextResult(updated), nil, nil
395+
}
396+
397+
func getCustomPropertyDefinitions(ctx context.Context, client *github.Client, apiURL, errorMessage string) ([]map[string]any, *mcp.CallToolResult) {
398+
req, err := client.NewRequest(ctx, http.MethodGet, apiURL, nil)
399+
if err != nil {
400+
return nil, utils.NewToolResultErrorFromErr("failed to create custom properties request", err)
401+
}
402+
403+
var properties []map[string]any
404+
resp, err := client.Do(req, &properties)
405+
if resp != nil {
406+
defer func() { _ = resp.Body.Close() }()
407+
}
408+
if err != nil {
409+
return nil, ghErrors.NewGitHubAPIErrorResponse(ctx, errorMessage, resp, err)
410+
}
411+
return properties, nil
412+
}
413+
414+
func validateUniqueCustomPropertyNames(properties []map[string]any) *mcp.CallToolResult {
415+
seen := make(map[string]struct{}, len(properties))
416+
for i, property := range properties {
417+
name := property["property_name"].(string)
418+
if _, ok := seen[name]; ok {
419+
return utils.NewToolResultError(fmt.Sprintf("properties[%d].property_name duplicates %q", i, name))
420+
}
421+
seen[name] = struct{}{}
422+
}
423+
return nil
424+
}
425+
426+
func mergeCustomPropertyDefinitions(current, requested []map[string]any, sourceType string) ([]map[string]any, *mcp.CallToolResult) {
427+
currentByName := make(map[string]map[string]any, len(current))
428+
for _, property := range current {
429+
name, _ := property["property_name"].(string)
430+
if name != "" {
431+
currentByName[name] = property
432+
}
433+
}
434+
435+
merged := make([]map[string]any, 0, len(requested))
436+
for i, update := range requested {
437+
name := update["property_name"].(string)
438+
existing, exists := currentByName[name]
439+
if !exists {
440+
if _, ok := update["value_type"]; !ok {
441+
return nil, utils.NewToolResultError(fmt.Sprintf("properties[%d].value_type is required for new property %q", i, name))
442+
}
443+
merged = append(merged, update)
444+
continue
445+
}
446+
if existingSource, _ := existing["source_type"].(string); existingSource != "" && existingSource != sourceType {
447+
return nil, utils.NewToolResultError(fmt.Sprintf("property %q is inherited from %s and cannot be updated at the %s level", name, existingSource, sourceType))
448+
}
449+
450+
property := map[string]any{"property_name": name}
451+
for _, field := range customPropertyDefinitionFields {
452+
if value, ok := existing[field]; ok {
453+
property[field] = value
454+
}
455+
if value, ok := update[field]; ok {
456+
property[field] = value
457+
}
458+
}
459+
merged = append(merged, property)
356460
}
461+
return merged, nil
357462
}
358463

359464
func parseCustomProperties[T any](args map[string]any, itemSchema *jsonschema.Schema) ([]T, *mcp.CallToolResult) {

0 commit comments

Comments
 (0)