feat(governance): add repository ruleset tools with multi-level scope challenge - #2991
feat(governance): add repository ruleset tools with multi-level scope challenge#2991SamMorrowDrums wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a non-default governance toolset for repository, organization, and enterprise rulesets.
Changes:
- Adds five ruleset read/create tools.
- Adds enterprise scopes and governance icon metadata.
- Adds tests, snapshots, and generated documentation.
Show a summary per file
| File | Description |
|---|---|
README.md |
Documents the governance toolset and tools. |
docs/remote-server.md |
Documents the remote governance endpoint. |
pkg/scopes/scopes.go |
Adds enterprise OAuth scopes. |
pkg/octicons/required_icons.txt |
Adds the law icon requirement. |
pkg/github/tools.go |
Registers governance metadata and tools. |
pkg/github/rulesets.go |
Implements ruleset tools and API operations. |
pkg/github/rulesets_test.go |
Tests ruleset schemas and handlers. |
pkg/github/__toolsnaps__/repository_ruleset_read.snap |
Snapshots repository read schema. |
pkg/github/__toolsnaps__/organization_repository_ruleset_read.snap |
Snapshots organization read schema. |
pkg/github/__toolsnaps__/create_repository_ruleset.snap |
Snapshots repository creation schema. |
pkg/github/__toolsnaps__/create_organization_repository_ruleset.snap |
Snapshots organization creation schema. |
pkg/github/__toolsnaps__/create_enterprise_repository_ruleset.snap |
Snapshots enterprise creation schema. |
Review details
- Files reviewed: 12/14 changed files
- Comments generated: 4
- Review effort level: Balanced
53de049 to
f340ea4
Compare
There was a problem hiding this comment.
Review details
Suppressed comments (12)
pkg/github/rulesets.go:780
- This validation only checks that each rule type survived. For a recognized type, unknown/misspelled
parametersfields are silently dropped by the typed JSON round-trip; unknownconditionsfields are likewise ignored. The request can therefore create a materially weaker or broader ruleset than requested. Validate the complete round-tripped payload (including nested fields, ordering/multiplicity, conditions, and bypass actors), or expose strict schemas for these structures.
// github.RepositoryRulesetRules.UnmarshalJSON silently discards rule types it
// does not recognize, which would let a typo create a weaker ruleset than the
// caller requested. Verify every requested rule type survived the round-trip.
pkg/github/rulesets.go:363
- Install the response-body close before checking
err; go-github can return a response with an error, and the current ordering leaks that body/connection.
rulesets, resp, err := client.Repositories.GetAllRulesets(ctx, owner, repo, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:379
- Install the response-body close before checking
err; otherwise failed branch-rule requests can leave the returned response body open.
branchRules, resp, err := client.Repositories.ListRulesForBranch(ctx, owner, repo, branch, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get repository rules for branch", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:455
client.Docan return bothrespanderr; because the defer is below the error return, failed rule-suite requests leak the response body. Close any non-nil response before the error check.
resp, err := client.Do(req, &ruleSuites)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list repository rule suites", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:475
client.Docan return bothrespanderr; install the close before the error return so failed rule-suite lookups do not leak the body/connection.
resp, err := client.Do(req, &ruleSuite)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get repository rule suite", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:487
- Defer closing a non-nil response before checking
err; organization API errors may still return a response body that otherwise remains open.
ruleset, resp, err := client.Organizations.GetRepositoryRuleset(ctx, org, rulesetID)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get organization repository ruleset", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:504
- Defer closing a non-nil response before checking
err; failed organization-list calls can return an open response body.
rulesets, resp, err := client.Organizations.ListAllRepositoryRulesets(ctx, org, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list organization repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:516
- Defer closing a non-nil response before checking
err; enterprise API errors may include a response body that must be closed.
ruleset, resp, err := client.Enterprise.GetRepositoryRuleset(ctx, enterprise, rulesetID)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get enterprise repository ruleset", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:547
- Move response cleanup ahead of the error return.
client.Docan return a non-nil response on an API error, so this ordering leaks failed enterprise-list responses.
resp, err := client.Do(req, &rulesets)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list enterprise repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:613
- Defer closing a non-nil response before the error check; failed repository create calls can return an open response body.
created, resp, err := client.Repositories.CreateRuleset(ctx, owner, repo, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:624
- Defer closing a non-nil response before the error check; otherwise organization create failures can leak their response body/connection.
created, resp, err := client.Organizations.CreateRepositoryRuleset(ctx, org, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create organization repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:635
- Defer closing a non-nil response before the error check; otherwise enterprise create failures can leak their response body/connection.
created, resp, err := client.Enterprise.CreateRepositoryRuleset(ctx, enterprise, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create enterprise repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
- Files reviewed: 13/15 changed files
- Comments generated: 3
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (15)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/github/rulesets.go:70
- This makes the write tool visible to classic PATs even when they have none of the scopes needed by any level. Unlike the read tool, creation has no public unauthenticated path; use ANY-of visibility for
repo,admin:org, oradmin:enterpriseso unsupported tools are filtered out, and update the visibility assertion accordingly.
func([]string) bool { return true },
pkg/github/rulesets.go:47
- The callback is case-sensitive, but the handler later dispatches with
strings.ToLower(level). A call withlevel: "Repository"therefore skips the OAuth challenge and still reaches the repository API. Normalize here as well (or reject mixed case in the handler) so accepted calls cannot bypass up-scoping.
switch level {
pkg/github/rulesets.go:76
- The handler accepts mixed-case levels via
strings.ToLower(level), while this pre-handler scope callback does not. For example,level: "Enterprise"reaches enterprise creation without producing the requiredadmin:enterprisechallenge. Apply the same normalization in both paths.
switch level {
pkg/github/rulesets.go:343
- Close the response body before checking
err. go-github can return a non-nil response with an open body on API errors, so the current early return leaks the connection instead of making it reusable.
ruleset, resp, err := client.Repositories.GetRuleset(ctx, owner, repo, rulesetID, includesParents)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get repository ruleset", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:363
- This returns before closing the response body on GitHub API errors. Defer closure immediately after the client call so failed list requests do not leak transport connections.
rulesets, resp, err := client.Repositories.GetAllRulesets(ctx, owner, repo, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:379
- An error response may still contain an open body, but this path returns before the defer is installed. Close non-nil responses before checking
errto avoid leaking connections.
branchRules, resp, err := client.Repositories.ListRulesForBranch(ctx, owner, repo, branch, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get repository rules for branch", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:455
client.Docan return both an error and a response whose body must be closed. Install the guarded defer before the error return so repeated failed rule-suite calls do not exhaust idle connections.
resp, err := client.Do(req, &ruleSuites)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list repository rule suites", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:475
- The error path returns before closing a non-nil response body from
client.Do. Move a nil-guarded defer ahead of the error check to preserve HTTP connection reuse.
resp, err := client.Do(req, &ruleSuite)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get repository rule suite", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:487
- A failed organization ruleset lookup can still return a response with an open body. Guard and defer the close before checking
errso the error path does not leak the connection.
ruleset, resp, err := client.Organizations.GetRepositoryRuleset(ctx, org, rulesetID)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get organization repository ruleset", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:504
- The response body is only closed on success. Since go-github returns responses for HTTP errors too, defer a guarded close before the early return.
rulesets, resp, err := client.Organizations.ListAllRepositoryRulesets(ctx, org, opts)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list organization repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:516
- This early error return leaves the response body open when GitHub returns an HTTP error. Install the nil-guarded defer before checking
err.
ruleset, resp, err := client.Enterprise.GetRepositoryRuleset(ctx, enterprise, rulesetID)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get enterprise repository ruleset", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:547
client.Domay return a non-nil response on failure, but the current return bypasses body closure. Move a guarded defer before the error check to avoid connection leaks.
resp, err := client.Do(req, &rulesets)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to list enterprise repository rulesets", resp, err), nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:613
- Repository creation returns before closing response bodies attached to GitHub API errors. Defer a guarded close immediately after the call so failed create attempts do not leak transport resources.
created, resp, err := client.Repositories.CreateRuleset(ctx, owner, repo, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:624
- The organization create error path skips response-body closure. A non-nil error response must be closed before returning to keep the HTTP transport reusable.
created, resp, err := client.Organizations.CreateRepositoryRuleset(ctx, org, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create organization repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
pkg/github/rulesets.go:635
- The enterprise create path leaks response bodies on API errors because the defer is installed only after the error check. Close any non-nil response before returning.
created, resp, err := client.Enterprise.CreateRepositoryRuleset(ctx, enterprise, ruleset)
if err != nil {
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to create enterprise repository ruleset", resp, err), nil, nil
}
defer func() { _ = resp.Body.Close() }()
- Files reviewed: 13/15 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (5)
Previously missed (4) — in code that hasn't changed since the last review.
pkg/github/rulesets.go:53
- The organization and enterprise GET ruleset endpoints require
admin:organdadmin:enterprise, respectively, for OAuth apps and classic PATs;read:org/read:enterpriseare insufficient. These challenges can therefore complete successfully with a token that the API immediately rejects with 403. Challenge for the admin scopes here and update the exhaustive scope list, tests, snapshots, and generated docs accordingly.
case "organization":
return scopes.ChallengeAll(activeScopes, scopes.ReadOrg)
case "enterprise":
return scopes.ChallengeAll(activeScopes, scopes.ReadEnterprise)
pkg/github/rulesets.go:396
- The rule-suite endpoint also supports the documented
evaluate_statusfilter (all,active, orevaluate), but this filter model omits it while exposing every other endpoint filter. Callers consequently cannot restrict results to evaluation-mode or active rulesets. Add it to the input schema, argument parsing, query construction, and regression coverage.
// ruleSuiteFilters holds the optional filters for listing rule suites.
type ruleSuiteFilters struct {
Ref string
TimePeriod string
ActorName string
RuleSuiteResult string
}
pkg/github/rulesets.go:456
- Decoding an untyped response into
anyconverts every JSON number tofloat64. Rule-suite IDs and actor IDs are 64-bit values, so values above 2^53 are rounded whenMarshalledTextResultencodes them again. Decode intojson.RawMessageto preserve the API response exactly.
This issue also appears on line 478 of the same file.
var ruleSuites any
pkg/github/rulesets.go:558
- Enterprise ruleset objects contain 64-bit numeric IDs, but decoding into
anyconverts them tofloat64and can change their values when the result is re-marshaled. Usejson.RawMessageso the direct API response retains integer precision.
var rulesets any
pkg/github/rulesets.go:478
- This untyped decode converts 64-bit IDs in the rule-suite response to
float64, which silently rounds values above 2^53 before returning them to the caller. Preserve the raw JSON instead.
var ruleSuite any
- Files reviewed: 13/15 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/github/rulesets.go:70
- The write tool is visible to every classic PAT, including tokens with none of the scopes that can authorize any of its operations. This defeats the startup filtering documented in
docs/scope-filtering.md:9-11and leaves users with an unusable write tool. Make visibility true only when the PAT has at least one ofrepo,admin:org, oradmin:enterprise, and update the shared visibility assertion in the scope tests accordingly.
func([]string) bool { return true },
- Files reviewed: 13/15 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
pkg/github/rulesets.go:379
- This forwards the branch name as a raw path segment. Valid branch names commonly contain
/(for examplefeature/login), so go-github builds/rules/branches/feature/logininstead of encoding the branch asfeature%2Flogin, and the endpoint does not match the requested branch. Escape the branch before passing it to this go-github method and add a slash-containing regression case.
branchRules, resp, err := client.Repositories.ListRulesForBranch(ctx, owner, repo, branch, opts)
pkg/github/rulesets.go:738
- Unknown top-level arguments are silently ignored when this map is converted into the outbound payload. Since this repository registers the raw
mcp.AddToolhandler and only unmarshals arguments into a map, the input schema does not reject additional properties; for example,conditioninstead ofconditionscreates the ruleset without any applicability condition. Reject unrecognized keys before constructing this governance request.
func buildRepositoryRulesetFromArgs(args map[string]any) (github.RepositoryRuleset, *mcp.CallToolResult) {
pkg/github/rulesets.go:764
- Only
typeandparameterssurvive go-github's ruleset-rule unmarshal, but extra keys on each rule object are not checked. A common typo such asparameter(singular) is therefore discarded and apull_requestrule is sent with zero/default parameters, potentially creating a weaker rule than requested. Reject every rule-object key other thantypeandparameters, as is already done for bypass actors.
for _, rule := range rules {
ruleMap, ok := rule.(map[string]any)
if !ok {
return github.RepositoryRuleset{}, utils.NewToolResultError("each rule must be an object with a 'type' field")
}
- Files reviewed: 13/15 changed files
- Comments generated: 0 new
- Review effort level: Balanced
… challenge Reimplements the repository ruleset support from #821 (issue #820) onto the current inventory-based tool architecture, consolidated into two level-aware tools in a new non-default `governance` toolset. - `repository_ruleset_read`: read rulesets, branch rules, and rule suites at repository, organization, or enterprise level. - `create_repository_ruleset`: create a ruleset at any of the three levels. A `level` argument selects the scope, and a DynamicChallenge up-scopes the required OAuth scope accordingly (repo -> read:org/admin:org -> read:enterprise/admin:enterprise), so the default surface only asks for repo scope. Ruleset creation round-trips the request through go-github's RepositoryRuleset unmarshalling and rejects rule types, parameters, conditions, or bypass-actor keys that are silently dropped, preventing typos from creating a weaker-than-intended ruleset. 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
8e78920 to
fe0ece5
Compare
Summary
Takes over #821 (issue #820) and reimplements repository ruleset support on the current inventory-based tool architecture. Rulesets and custom properties are split into a 2-PR stack; this is PR 1 of 2 (rulesets + the new
governancetoolset). Custom properties follow in the stacked PR.The original PR predates the current SDK (it was written against
mark3labs/mcp-go+ an old go-github) and had grown to ~16 separate tools across repo/org/enterprise levels. This reimplementation consolidates them.What's here
A new non-default
governancetoolset with two level-aware tools:repository_ruleset_readcreate_repository_rulesetA
levelargument selects the scope. Instead of separate org/enterprise tools each demanding elevated scope up front, a DynamicChallenge up-scopes the required OAuth scope based on the chosen level:reporead:org/admin:orgread:enterprise/admin:enterpriseSo the default surface only ever asks for
repo, and the broader scopes are challenged for on demand when a caller actually targets an org or enterprise.Safety: silent-drop protection on create
go-github'sRepositoryRulesetunmarshalling silently discards rule types, rule parameters, condition keys, and bypass-actor keys it doesn't recognise. For a governance tool that's a real footgun — a typo (require_code_owners_reviewvsrequire_code_owner_review) would create a weaker ruleset than requested without any error.create_repository_rulesetround-trips the request and rejects anything that didn't survive, so typos fail loudly instead of silently downgrading protection.Placement
The original PR placed repo-level tools in the default
repostoolset and added an always-onenterprisetoolset. This keeps the default surface lean by putting everything in a dedicated non-defaultgovernancetoolset instead.Notes
admin:org,read:enterprise,admin:enterprise.Co-authored with the original author, @patrick-knight.