feat(governance): add ruleset tools with dynamic scope challenges - #2991
feat(governance): add ruleset tools with dynamic scope challenges#2991SamMorrowDrums wants to merge 6 commits 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
Add a new non-default "governance" toolset (icon: law) with tools for managing GitHub repository rulesets at the repository, organization, and enterprise levels. Read operations are consolidated behind method-dispatch tools to match the current MCP surface: - repository_ruleset_read (get, list, get_rules_for_branch, list_rule_suites, get_rule_suite) - organization_repository_ruleset_read (get, list) Write operations remain single-purpose tools, split by level because each level requires a distinct OAuth scope for scope-challenge accuracy: - create_repository_ruleset (repo) - create_organization_repository_ruleset (admin:org) - create_enterprise_repository_ruleset (admin:enterprise) Adds the read:enterprise and admin:enterprise scopes and the law octicon as shared governance infrastructure. Supersedes #821. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1e886867-a922-419a-b02c-ac643716aea8
…allenges Rebase onto main (which now includes #3128's per-call OAuth scope checks) and redesign the ruleset tools around that API instead of the old tool-name-keyed challenge middleware that motivated splitting by level. - Collapse the 5 level-specific tools (repository_ruleset_read, organization_repository_ruleset_read, create_repository_ruleset, create_organization_repository_ruleset, create_enterprise_repository_ruleset) into 2: repository_ruleset_read and create_repository_ruleset. Both take a "level" argument (repository/organization/enterprise) and use scopes.DynamicChallenge to return the exact repo / read:org / admin:org / read:enterprise / admin:enterprise challenge for that call, using the scope hierarchy so a broader granted scope still satisfies the challenge. - A missing or unrecognized "level" (or a non-string value) returns no challenge so normal handler argument validation produces the error, instead of prompting for scopes on a malformed call. - Keep repository_ruleset_read and create_repository_ruleset as separate tools since ReadOnlyHint-based read-only filtering depends on that split. - Add enterprise-level "get" and "list" read support (list issued directly via GET /enterprises/{enterprise}/rulesets, matching the existing rule suite pattern, since go-github has no typed wrapper for it) so all three levels have symmetric read coverage. - Re-add read:enterprise/admin:enterprise to pkg/scopes as opt-in (non-default) OAuth scopes, alongside admin:org which no tool had previously requested, and register them in oauthScopeDefinitions so they are advertised in OAuth protected-resource metadata. - Fetch the "law" toolset icon into the new icons_data_uris.txt embed (required_icons.txt already listed it) and bump go-github v87 -> v89 to match main. - Update README/toolsnaps via script/generate-docs and UPDATE_TOOLSNAPS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Address four Copilot review comments on the consolidated ruleset tools: - Verify caller-supplied rule *parameters* survive the round-trip through github.RepositoryRulesetRules, not just rule *types*. go-github silently drops unrecognized parameter keys during JSON unmarshal (e.g. a "require_code_owners_review" typo for "require_code_owner_review" becomes the weaker default false), so a supplied parameter that vanishes after the round-trip is now rejected by name. A caller-supplied zero value (false, 0, "", empty array/object) is exempted from this check since it's indistinguishable from an omitempty-dropped field. - Reject rulesets with two rules of the same type up front, since RepositoryRulesetRules has one field per type and a duplicate would silently overwrite the first rule during the round-trip rather than erroring or producing two rules. - Add the "exempt" bypass_mode (alongside the existing "always" and "pull_request") to bypass_actors, matching the repository, organization, and enterprise ruleset APIs. - Add "User", "EnterpriseOwner", and "EnterpriseRole" to the bypass_actors actor_type enum. The consolidated create_repository_ruleset tool shares one schema across all three levels, so the enum is now the union of every level's valid actor types (repository and organization already matched; only enterprise adds EnterpriseOwner/EnterpriseRole), with the enterprise-only values called out in the description. An invalid combination (e.g. EnterpriseOwner at the repository level) is left to the GitHub API to reject, consistent with how this tool already defers other cross-field validation. - Add HasAll/ChallengeAll coverage for AdminEnterprise -> ReadEnterprise scope hierarchy resolution (pkg/scopes/scopes_test.go already asserted the static ScopeHierarchy map entry). Regenerated toolsnaps for create_repository_ruleset. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Address a second round of Copilot review feedback found on the consolidated
ruleset tools:
- Security fix: the "level" dispatch in both tool handlers lowercased the
argument (strings.ToLower(level)) before matching it, while the scope
challenge in rulesetReadScopeAccess/rulesetWriteScopeAccess matches "level"
with an exact, case-sensitive comparison. A caller could send
level: "Organization" (or "Enterprise") and reach the organization/
enterprise-level API call while the OAuth middleware -- which challenges on
the same raw argument -- found no case match and required no admin:org /
admin:enterprise scope at all. Handler dispatch on "level" is now
case-sensitive (matching the schema's lowercase enum exactly), so a
mismatched-case value now falls through to "unknown level" in both tools
instead of silently executing a privileged call unscoped. "method" case
normalization is untouched since it has no bearing on scope selection.
Added regression tests in both Test_RepositoryRulesetRead and
Test_CreateRepositoryRuleset that assert the API is never called and the
scope challenge independently agrees.
- Fixed 12 instances where a GitHub API response's body/connection leaked on
the error path: defer resp.Body.Close() was installed only after the
err != nil check returned, so a non-nil response accompanying an error was
never closed. Every call site now guards `if resp != nil { defer ... }`
before checking err, matching the existing FetchRepoIsPrivate pattern in
pkg/github/repositories.go.
- Generalized the rule-parameter round-trip check into a recursive
droppedKeyPath helper and applied it to `conditions` as well as
rules[].parameters. github.RepositoryRulesetConditions silently drops
unrecognized keys the same way github.RepositoryRulesetRules does (e.g. a
"ref_names" typo for "ref_name", or a nested "includes" typo for
"include"), so an unrecognized condition key at any nesting depth is now
rejected by name instead of silently producing a ruleset with weaker or no
applicability conditions than requested. Only nested objects are recursed
into; array elements stay opaque so API-side reordering can't produce a
false positive.
Note: committed unsigned -- the local 1Password SSH-signing agent was
unavailable (session locked) at commit time.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Address the remaining Copilot review comment: github.BypassActor only recognizes actor_id, actor_type, and bypass_mode, and JSON unmarshal silently discards any other key (e.g. a "bypass_modes" typo). Because the API defaults an omitted bypass_mode to "always", a dropped key would silently grant the actor broader bypass rights than requested. Since BypassActor is a small, fixed set of keys (unlike rule parameters or conditions, which vary per rule/condition type and need the round-trip check), each bypass_actors[i] is now validated directly against an allow-list of the three recognized keys before the request is built, and the unrecognized key is named in the error along with its array index. Added a regression test with a "bypass_modes" typo, and confirmed the existing valid-input test (EnterpriseOwner/exempt) still passes. Note: committed unsigned -- the local 1Password SSH-signing agent is still unavailable (session locked). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Address the final Copilot review comment: droppedKeyPath previously treated array elements as opaque, so an unrecognized key inside an array element (e.g. required_status_checks[].integration_ids, a typo for integration_id) was silently dropped by go-github's JSON unmarshal without being caught -- the resulting rule would then accept a status check from any integration instead of only the one requested. This round-trip is entirely local (our own marshal/unmarshal of a go-github struct, not a remote API response), so slice order and length are deterministic and safe to compare by index. droppedKeyPath now delegates to a new droppedValuePath helper that recurses into both nested objects and array elements, reporting paths like "required_status_checks[0].integration_ids" when a caller-supplied key disappears in either a map or an array position. Added a regression test for the array-nested typo, and a companion test confirming valid array-nested parameters still round-trip without being misflagged. Note: committed unsigned -- the local 1Password SSH-signing agent is still unavailable (session locked). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
Governance toolset — Rulesets
Adds a new non-default
governancetoolset (icon:law) with GitHub repository rulesets tools spanning the repository, organization, and enterprise levels.This supersedes the very stale #821 (re: #820).
Tools (2)
levelvaluesrepository_ruleset_readrepository,organization,enterpriseget,listat every level;get_rules_for_branch,list_rule_suites,get_rule_suiteat repository level onlycreate_repository_rulesetrepository,organization,enterpriseEach tool challenges for the exact OAuth scope implied by the call's
level:levelrepositoryreporepoorganizationread:orgadmin:orgenterpriseread:enterpriseadmin:enterpriseDesign notes
This PR was rebased onto current
main, which now includes #3128's per-call OAuth scope checks (scopes.DynamicChallenge, argument-awareChallenge/Visiblecallbacks keyed off the tool, not a static per-tool scope list). That superseded the reason the original version of this PR split rulesets into 5 level-specific tools: the old challenge middleware could only key on tool name, so a single tool couldn't request different scopes for different calls.With the new API:
repository_ruleset_read,create_repository_ruleset), taking alevelargument (repository/organization/enterprise).scopes.DynamicChallengeinspectslevelat call time and returns the exact scope for that call (using the scope hierarchy, so a broader granted scope still satisfies the challenge). A missing or unrecognizedlevel(or a non-string value) returns no challenge, so normal handler argument validation produces the error instead of prompting for scopes on a malformed call.ReadOnlyHintstill drives read-only-mode filtering — that boundary encodes real behavior and isn't affected by the scope-challenge change.getandlist, the latter via a directGET /enterprises/{enterprise}/rulesetsrequest since go-github has no typed wrapper, matching the existing rule-suite pattern) — the original PR only had enterprisecreate.read:enterprise/admin:enterprise(and registers previously-unusedadmin:org) as opt-in OAuth scopes, and thelawocticon, as shared governance infrastructure.Verified:
script/lint(0 issues),script/test,script/generate-docsall green.Related