diff --git a/.github/skills/create-mcp-tool/SKILL.md b/.github/skills/create-mcp-tool/SKILL.md index b4a67db19..bacea9685 100644 --- a/.github/skills/create-mcp-tool/SKILL.md +++ b/.github/skills/create-mcp-tool/SKILL.md @@ -77,6 +77,7 @@ Rules: - Tag fields that only exist at a higher GitLab tier with `tier:"premium"` or `tier:"ultimate"`; the catalog prunes them from the schema below that tier - Use `jsonschema:"description,required"` for a field GitLab requires on the route the action sends, and only for that: the marker is the one required list all three surfaces serve (`omitempty` no longer decides it), and `make audit-1to1-required` fails on a marker the live record disagrees with unless `cmd/audit_1to1/internal/required/declarations.go` says why. A choice between fields GitLab states as `at_least_one_of` is an `anyOf` through `toolutil.SchemaAnyOfRequired` in the spec's `InputSchemaOverrides`, and one it states as `exactly_one_of` a `oneOf` through `toolutil.SchemaOneOfRequired`, with neither field marked; only the SDK's schema validation holds a call to one `oneOf` branch, so the handler of an `exactly_one_of` also refuses a call naming two of the fields before it sends anything, as `groupldap`'s do. A rule on each element of a list is an items override on that list (`toolutil.SchemaItemsOneOfRequired`, `SchemaItemsAnyOfRequired`, `SchemaItemsAnyOfRequiredUnlessTrue` and `SchemaItemsAtMostOneOf`, built on `toolutil.SchemaItemsOverride`), and since neither the meta nor the dynamic dispatcher reads an item schema, the handler refuses an element that breaks it, as the protected environment protect and update do through `toolutil.CheckEnvironmentProtect` and `CheckEnvironmentUpdate` - Use `json:",omitempty"` for optional fields +- A membership access level parameter publishes the levels its route accepts and no other, from `toolutil.AccessLevelSet` in `internal/toolutil/access_level.go` (`ProjectAccessLevels`, `GroupAccessLevels` and the rest, read from GitLab's routes and models): `SchemaOverride` (or `SchemaItemsOverride` for a list) in the spec's `InputSchemaOverrides` makes it an integer enum, and its `jsonschema` description lists the same levels as `N=Role`. `min_access_level` and `shared_min_access_level` get theirs centrally. `TestBuildActionCatalog_MembershipAccessLevels_OfferOnlyWhatTheRouteAccepts` in `internal/tools` fails on such a parameter it does not map to a set, and on a description that offers a level outside it - A string whose form GitLab checks publishes that form, so a model knows what to send and find's example fills it with a value of it rather than `"value"`: a `format` through `toolutil.SchemaFormatOverride` (dates, URLs and `email` get theirs centrally, on a plain string only, so a nullable or nested one is overridden per action), or a `pattern` through `toolutil.SchemaPatternOverride`, with `toolutil.CronPattern` for a cron expression and `toolutil.FullCommitSHAPattern` for a SHA where GitLab holds the parameter to a full one. A SHA that also takes a ref name, a duration, and a field cleared with an empty string get no pattern, since it would refuse values GitLab takes, and a string that takes several values in one, an invitation's comma-separated emails or an OAuth application's redirect URIs one per line, gets no format, since a format names one value: `toolutil.SchemaNoFormatOverride` keeps a central one off it. `TestFind_FormatBearingParams_DeclareTheirFormatAndExampleIt` in `internal/tools/dynamic` holds every required parameter of the catalog to it - No domain prefix on type names — the package provides namespace diff --git a/CLAUDE.md b/CLAUDE.md index b8174d5c1..165a2502d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -701,7 +701,7 @@ go test ./internal/prompts/ -count=1 -v # Prompts ### Running E2E tests -The suite drives the **real `cmd/server` binary** over stdio against a real GitLab, so a tool call crosses a process boundary and then the network; read `test/e2e/README.md` before changing any of it. **The package decides the runtime, not a build tag**: `test/e2e/gitlab/common` for what any instance serves (175 test files), `ce` for what only an unlicensed one does (5), `ee` for Premium and Ultimate (48), all three compiled together under the one `e2e` tag. Every target stages the binary first, so none of them is a bare `go test`: +The suite drives the **real `cmd/server` binary** over stdio against a real GitLab, so a tool call crosses a process boundary and then the network; read `test/e2e/README.md` before changing any of it. **The package decides the runtime, not a build tag**: `test/e2e/gitlab/common` for what any instance serves (176 test files), `ce` for what only an unlicensed one does (6), `ee` for Premium and Ultimate (49), all three compiled together under the one `e2e` tag. Every target stages the binary first, so none of them is a bare `go test`: ```bash make test-e2e # self-hosted, from .env (GITLAB_URL, GITLAB_TOKEN); a package the instance cannot serve skips diff --git a/cmd/audit_1to1/internal/enums/exemptions.go b/cmd/audit_1to1/internal/enums/exemptions.go index 2b5a41bed..c31f12b64 100644 --- a/cmd/audit_1to1/internal/enums/exemptions.go +++ b/cmd/audit_1to1/internal/enums/exemptions.go @@ -9,26 +9,33 @@ import "maps" // while GitLab documents what each endpoint accepts, so the same type-wide // constant is excused on many fields for the same documented reason. const ( + // The membership reasons below cite the routes and models at v19.5.0-ee, + // which register row 110 of docs/development/upstream-bugs.md records, + // rather than the API pages, which listed levels the routes refuse until + // the documentation change that row names. The schemas offer what + // toolutil.AccessLevelSet holds (issue 1280). + // // membershipNoAdmin: 60 (Admin) is a constant on AccessLevelValue because // a protected ref can be restricted to administrators, not because a // membership can carry it. - membershipNoAdmin = "60 (Admin) is not a level a membership can carry: doc/api/project_members.md, group_members.md, invitations.md and access_requests.md list 0 (No access) through 50 (Owner) for adding, editing, inviting and approving; only the group member edit endpoint documents 60, and that field offers it" + membershipNoAdmin = "60 (Admin) is not a level a membership can carry: no member, invitation, access request, share or group link route accepts it at v19.5.0-ee, the group member update included (docs/development/upstream-bugs.md row 110)" // membershipNoAccessGrant: 0 (No access) is a level a membership reads - // back with, not one the add and edit endpoints grant; the handlers treat - // 0 as the field left unset and refuse the request before it leaves. - membershipNoAccessGrant = "0 (No access) is read back on a membership but never granted: doc/api/members.md lists it under valid access levels while the add and edit handlers (members.go, group_members.go) treat 0 as access_level left unset and refuse the call" + // back with, not one any write grants. + membershipNoAccessGrant = "0 (No access) is read back on a membership but never granted: no write route accepts it at v19.5.0-ee, and an access request approval answers it with 201 while the request stays pending, so the approvals refuse it before the request (docs/development/upstream-bugs.md row 110)" + // projectNoMinimalAccess: Minimal access is a group level. + projectNoMinimalAccess = "5 (Minimal access) is refused on a project in every edition at v19.5.0-ee: a project member, invitation, share and approval are held to Gitlab::Access.all_values, Guest (10) to Owner (50) (docs/development/upstream-bugs.md row 110)" + // groupLinkNoAccess: a SAML or LDAP group link grants a role. + groupLinkNoAccess = "the SAML and LDAP group link routes declare values_with_minimal_access at v19.5.0-ee, Minimal access (5) to Owner (50), so 0 (No access) is refused though doc/api/saml.md and group_ldap_links.md list it (docs/development/upstream-bugs.md row 110)" // memberRoleBaseLevels: a custom role is based on a role a member can - // hold, which leaves out both ends of AccessLevelValue and the minimal - // access level in between. - memberRoleBaseLevels = "doc/api/member_roles.md#add-a-member-role-to-an-instance lists 10 (Guest) through 50 (Owner) as the valid base access levels" - // broadcastTargetLevels: the roles a banner can target are the membership - // roles proper, without the two ends of AccessLevelValue. - broadcastTargetLevels = "doc/api/broadcast_messages.md lists target_access_levels as 10 (Guest), 15 (Planner), 20 (Reporter), 25 (Security Manager), 30 (Developer), 40 (Maintainer) and 50 (Owner); 5 (Minimal access) and 60 (Admin) are AccessLevelValue constants outside that set" - // minAccessLevelFilter: the list filters document the membership roles a - // caller can hold, which excludes both ends of the type. - minAccessLevelFilter = "doc/api/groups.md and projects.md document min_access_level and shared_min_access_level as 5 (Minimal access) through 50 (Owner): 0 (No access) filters nothing and 60 (Admin) is not a membership level" + // hold, below Owner. + memberRoleBaseLevels = "the create route declares Gitlab::Access.all_values and MemberRole validates the level against options_for_custom_roles at v19.5.0-ee, so Guest (10) to Maintainer (40) is what both accept: 0, 5 (Minimal access), 50 (Owner) and 60 (Admin) are each refused by one of the two, though doc/api/member_roles.md lists 50 (docs/development/upstream-bugs.md row 110)" + // broadcastTargetLevels: the roles a banner can target. + broadcastTargetLevels = "the routes declare target_access_levels with values: System::BroadcastMessage::ALLOWED_TARGET_ACCESS_LEVELS at v19.5.0-ee, Guest, Planner, Reporter, Developer, Maintainer and Owner; 0, 5 (Minimal access), 25 (Security Manager) and 60 (Admin) are AccessLevelValue constants outside it, though doc/api/broadcast_messages.md lists 25 (docs/development/upstream-bugs.md row 110)" + // minAccessLevelFilter: the list filters take the membership roles a + // caller can hold. + minAccessLevelFilter = "every min_access_level and shared_min_access_level route declares values: Gitlab::Access.all_values at v19.5.0-ee, Guest (10) to Owner (50) in every edition: 0 filters nothing, 5 (Minimal access) is refused and 60 (Admin) is not a membership level (docs/development/upstream-bugs.md row 110)" // shareAccessLevel covers the two share endpoints, which grant a role. - shareAccessLevel = "doc/api/groups.md#share-groups-with-groups and projects.md#share-a-project-with-a-group list 5 (Minimal access) through 50 (Owner); 0 grants nothing and 60 (Admin) is not a membership level" + shareAccessLevel = "a group share declares values_with_minimal_access and a project share Gitlab::Access.all_values at v19.5.0-ee: 0 grants nothing, 60 (Admin) is not a membership level, and a project share refuses 5 (Minimal access) as well (docs/development/upstream-bugs.md row 110)" // protectedBranchLevels is the four-value set every protection level // parameter documents, against the membership-wide AccessLevelValue. protectedBranchLevels = "doc/api/protected_branches.md#valid-access-levels lists 0 (No access, push and merge only), 30, 40 and 60 (GitLab Self-Managed only); AccessLevelValue is the membership-wide set, and the other levels are not protection levels" @@ -64,7 +71,8 @@ const ( // a subset of the SDK's type-wide constants. // - "..=" — one value, missing or extra. // -// Each entry carries a reason that cites the GitLab API doc, and an entry +// Each entry carries a reason that cites the GitLab API doc, or the route +// and model where the doc lists a value they refuse, and an entry // that excuses nothing (the field is gone, or the gap closed) is a finding, // so the table cannot outlive what it describes. var acceptedEnumGaps = buildAcceptedEnumGaps() @@ -75,50 +83,76 @@ var acceptedEnumGaps = buildAcceptedEnumGaps() func buildAcceptedEnumGaps() map[string]string { table := map[string]string{ // --- Membership access levels (AccessLevelValue) --- + "accessrequests.ApproveGroupInput.access_level=0": membershipNoAccessGrant, "accessrequests.ApproveGroupInput.access_level=60": membershipNoAdmin, + "accessrequests.ApproveProjectInput.access_level=0": membershipNoAccessGrant, + "accessrequests.ApproveProjectInput.access_level=5": projectNoMinimalAccess, "accessrequests.ApproveProjectInput.access_level=60": membershipNoAdmin, "groupmembers.AddInput.access_level=60": membershipNoAdmin, + "groupmembers.EditInput.access_level=60": membershipNoAdmin, + "invites.GroupInvitesInput.access_level=0": membershipNoAccessGrant, "invites.GroupInvitesInput.access_level=60": membershipNoAdmin, + "invites.ProjectInvitesInput.access_level=0": membershipNoAccessGrant, + "invites.ProjectInvitesInput.access_level=5": projectNoMinimalAccess, "invites.ProjectInvitesInput.access_level=60": membershipNoAdmin, + "members.AddInput.access_level=5": projectNoMinimalAccess, "members.AddInput.access_level=60": membershipNoAdmin, + "members.EditInput.access_level=5": projectNoMinimalAccess, "members.EditInput.access_level=60": membershipNoAdmin, "members.AddInput.access_level=0": membershipNoAccessGrant, "members.EditInput.access_level=0": membershipNoAccessGrant, "groupmembers.AddInput.access_level=0": membershipNoAccessGrant, "groupmembers.EditInput.access_level=0": membershipNoAccessGrant, - "groupldap.AddInput.group_access=60": "doc/api/group_ldap_links.md#add-an-ldap-group-link lists 0 (No access) through 50 (Owner); " + membershipNoAdmin, - "groupsaml.AddInput.access_level=60": "doc/api/saml.md#add-a-saml-group-link lists 0 (No access) through 50 (Owner); " + membershipNoAdmin, + "groupldap.AddInput.group_access=0": groupLinkNoAccess, + "groupldap.AddInput.group_access=60": membershipNoAdmin, + "groupsaml.AddInput.access_level=0": groupLinkNoAccess, + "groupsaml.AddInput.access_level=60": membershipNoAdmin, "memberroles.CreateInstanceInput.base_access_level=0": memberRoleBaseLevels, "memberroles.CreateInstanceInput.base_access_level=5": memberRoleBaseLevels, + "memberroles.CreateInstanceInput.base_access_level=50": memberRoleBaseLevels, "memberroles.CreateInstanceInput.base_access_level=60": memberRoleBaseLevels, "broadcastmessages.CreateInput.target_access_levels=0": broadcastTargetLevels, "broadcastmessages.CreateInput.target_access_levels=5": broadcastTargetLevels, + "broadcastmessages.CreateInput.target_access_levels=25": broadcastTargetLevels, "broadcastmessages.CreateInput.target_access_levels=60": broadcastTargetLevels, "broadcastmessages.UpdateInput.target_access_levels=0": broadcastTargetLevels, "broadcastmessages.UpdateInput.target_access_levels=5": broadcastTargetLevels, + "broadcastmessages.UpdateInput.target_access_levels=25": broadcastTargetLevels, "broadcastmessages.UpdateInput.target_access_levels=60": broadcastTargetLevels, - "groups.InvitedListInput.min_access_level=0": minAccessLevelFilter, - "groups.InvitedListInput.min_access_level=60": minAccessLevelFilter, - "groups.ListInput.min_access_level=0": minAccessLevelFilter, - "groups.ListInput.min_access_level=60": minAccessLevelFilter, - "groups.ListProjectsInput.min_access_level=0": minAccessLevelFilter, - "groups.ListProjectsInput.min_access_level=60": minAccessLevelFilter, - "groups.ListSharedProjectsInput.min_access_level=0": minAccessLevelFilter, - "groups.ListSharedProjectsInput.min_access_level=60": minAccessLevelFilter, - "groups.SharedWithListInput.min_access_level=0": minAccessLevelFilter, - "groups.SharedWithListInput.min_access_level=60": minAccessLevelFilter, - "groups.SubgroupsListInput.min_access_level=0": minAccessLevelFilter, - "projects.ListInvitedGroupsInput.min_access_level=0": minAccessLevelFilter, - "projects.ListProjectGroupsInput.shared_min_access_level=0": minAccessLevelFilter, + "groups.InvitedListInput.min_access_level=0": minAccessLevelFilter, + "groups.InvitedListInput.min_access_level=5": minAccessLevelFilter, + "groups.InvitedListInput.min_access_level=60": minAccessLevelFilter, + "groups.ListInput.min_access_level=0": minAccessLevelFilter, + "groups.ListInput.min_access_level=5": minAccessLevelFilter, + "groups.ListInput.min_access_level=60": minAccessLevelFilter, + "groups.ListProjectsInput.min_access_level=0": minAccessLevelFilter, + "groups.ListProjectsInput.min_access_level=5": minAccessLevelFilter, + "groups.ListProjectsInput.min_access_level=60": minAccessLevelFilter, + "groups.ListSharedProjectsInput.min_access_level=0": minAccessLevelFilter, + "groups.ListSharedProjectsInput.min_access_level=5": minAccessLevelFilter, + "groups.ListSharedProjectsInput.min_access_level=60": minAccessLevelFilter, + "groups.SharedWithListInput.min_access_level=0": minAccessLevelFilter, + "groups.SharedWithListInput.min_access_level=5": minAccessLevelFilter, + "groups.SharedWithListInput.min_access_level=60": minAccessLevelFilter, + "groups.SubgroupsListInput.min_access_level=0": minAccessLevelFilter, + "groups.SubgroupsListInput.min_access_level=5": minAccessLevelFilter, + "groups.SubgroupsListInput.min_access_level=60": minAccessLevelFilter, + "projects.ListInvitedGroupsInput.min_access_level=0": minAccessLevelFilter, + "projects.ListInvitedGroupsInput.min_access_level=5": minAccessLevelFilter, + "projects.ListInvitedGroupsInput.min_access_level=60": minAccessLevelFilter, + "projects.ListProjectGroupsInput.shared_min_access_level=0": minAccessLevelFilter, + "projects.ListProjectGroupsInput.shared_min_access_level=5": minAccessLevelFilter, + "projects.ListProjectGroupsInput.shared_min_access_level=60": minAccessLevelFilter, "groups.ShareGroupInput.group_access=0": shareAccessLevel, "groups.ShareGroupInput.group_access=60": shareAccessLevel, "groupmembers.ShareInput.group_access=0": shareAccessLevel, "groupmembers.ShareInput.group_access=60": shareAccessLevel, "projects.ShareProjectInput.group_access=0": shareAccessLevel, + "projects.ShareProjectInput.group_access=5": shareAccessLevel, "projects.ShareProjectInput.group_access=60": shareAccessLevel, // --- Protection levels (AccessLevelValue) --- diff --git a/cmd/audit_meta_descriptions/doc.go b/cmd/audit_meta_descriptions/doc.go index c6ed65daf..0a71110cf 100644 --- a/cmd/audit_meta_descriptions/doc.go +++ b/cmd/audit_meta_descriptions/doc.go @@ -20,15 +20,18 @@ // accepts; // - an enum value the description spells for a parameter whose routes publish // an enum that does not carry it; -// - a value the description spells for a parameter that publishes no enum but -// whose own schema description spells a value set of its own; +// - a value the description spells for a parameter that publishes no enum of +// strings but whose own schema description spells a value set of its own; // - a "Parameter guidance:" line written for a parameter the action it names // does not accept. // -// The third rule exists because a schema enum is a closed set of strings, so a -// numeric value set cannot be one. gitlab_member_role's base_access_level -// offered 5 in the prose while the schema description said 10 to 50, and -// doc/api/member_roles.md agrees with the schema: GitLab refuses 5. Both sides +// The third rule exists because the audit folds only the string values of a +// published enum, so a numeric value set, published as an integer enum (as +// every access level parameter is since issue 1280) or as none, is compared +// with nothing through the schema. gitlab_member_role's base_access_level +// offered 5 in the prose while the schema description said 10 to 50, the set +// doc/api/member_roles.md lists, and GitLab refuses 5 (its model refuses 50 +// as well, and the schema has said 10 to 40 since issue 1280). Both sides // of that comparison are read by one function, so the description's own // "10=Guest, 20=Reporter" list is the oracle and a set nothing spells is not // judged at all. diff --git a/cmd/audit_meta_descriptions/main.go b/cmd/audit_meta_descriptions/main.go index 10f0d1158..41203c641 100644 --- a/cmd/audit_meta_descriptions/main.go +++ b/cmd/audit_meta_descriptions/main.go @@ -4,13 +4,14 @@ package main import ( + "cmp" "encoding/json" "flag" "fmt" "io" "os" "regexp" - "sort" + "slices" "strings" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -107,14 +108,18 @@ type schemas struct { byAction map[string]accepted } +// osExit is os.Exit behind a variable, so a test can run main and read the +// code it would exit with. +var osExit = os.Exit + func main() { check := flag.Bool("check", false, "exit non-zero when a served description disagrees with the schemas") uncovered := flag.Bool("uncovered", false, "name the enumeration lines the extraction rule refused instead of only counting them") flag.Parse() - // os.Exit lives here, not in run: run is what the tests drive for its exit - // code, and an exit inside it would take the test binary with it. - os.Exit(run(*check, *uncovered)) + // The exit lives here, not in run: run is what most tests drive for its + // exit code, and an exit inside it would take the test binary with it. + osExit(run(*check, *uncovered)) } // run performs the audit and returns the process exit code. @@ -172,18 +177,18 @@ func audit(served []*mcp.Tool, catalog *actioncatalog.Catalog) (findings []findi } // The line is part of the key, and the sort is stable, because six lines of // one tool naming one wrong parameter tie on everything else: without it the - // same corpus printed those six in a different order from run to run. - sort.SliceStable(findings, func(i, j int) bool { - if findings[i].tool != findings[j].tool { - return findings[i].tool < findings[j].tool - } - if findings[i].kind != findings[j].kind { - return findings[i].kind < findings[j].kind - } - if findings[i].detail != findings[j].detail { - return findings[i].detail < findings[j].detail - } - return findings[i].line < findings[j].line + // same corpus printed those six in a different order from run to run. The + // key is compared with strings.Compare rather than with <, whose boundary + // mutant answers the same for every pair the comparator is asked about: two + // parts that differ order alike under < and <=, and two findings equal in + // every part are indistinguishable wherever they land. + slices.SortStableFunc(findings, func(a, b finding) int { + return cmp.Or( + strings.Compare(a.tool, b.tool), + strings.Compare(a.kind, b.kind), + strings.Compare(a.detail, b.detail), + strings.Compare(a.line, b.line), + ) }) return findings, lines, refused } @@ -192,9 +197,10 @@ func audit(served []*mcp.Tool, catalog *actioncatalog.Catalog) (findings []findi // // A spelled value is judged against the property's published enum where there // is one, and otherwise against the value set the property's own description -// spells. The second half exists because a numeric value set cannot be an enum -// of strings: gitlab_member_role's base_access_level offered 5 in the prose -// while its description said 10 to 50, and GitLab refuses 5. +// spells. The second half exists because only an enum's string values are +// folded (see addEnum), so a numeric value set, an integer enum included, is +// held to its description: gitlab_member_role's base_access_level offered 5 +// in the prose while its description said 10 to 50, and GitLab refuses 5. func judge(toolName string, line enumeration, allowed accepted) []finding { var found []finding for _, param := range line.params { @@ -344,8 +350,9 @@ func (a accepted) add(schema map[string]any) { } // addEnum folds one property's published enum into the accepted set, keeping -// only its string values: a schema enum is a closed set of strings, and a -// number in one is not a value any description spells. +// only its string values. A numeric set is spelled as value=label pairs, which +// labeledValues reads, so an integer enum, such as the one every access level +// parameter publishes, is left to the property's own description. func (a accepted) addEnum(name string, property map[string]any) { enum, ok := property["enum"].([]any) if !ok { @@ -533,10 +540,11 @@ func annotationValues(annotation string) []string { // labeledValues reads a "=