feat: Add a check-schema-fields command to the metadata tool

[JamBalaya56562]

Adds a check-schema-fields command to tools/metadata that checks Go struct JSON field optionality against GitHub's OpenAPI schemas — required, non-nullable schema fields must be non-pointer fields without an omit option, optional fields must remain omittable, and the field sets must line up. This grew out of @gmlewis's suggestion in #4319 and helps the pass-by-value work in #3644, where each conversion needs exactly this comparison done by hand today.

How structs are matched to schemas (reworked)

The original submission tried to match OpenAPI component schemas to structs automatically and could only do so unambiguously for 4 of 1142 schemas, as @stevehipwell and @alexandear pointed out. That matching is now gone. Instead, a struct opts in with explicit //meta:schema annotations in its doc comment, mirroring the existing //meta:operation convention:

// IssueCommentRequest represents a request to create or update an issue comment.
//
//meta:schema request POST /repos/{owner}/{repo}/issues/{issue_number}/comments
//meta:schema request PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}
type IssueCommentRequest struct {

• request/response selects the operation's request body or first 2xx JSON response schema; the operation is resolved against the downloaded descriptions (api.github.com, then ghec, then ghes — so GHES-only operations resolve too) using the same path normalization as //meta:operation.
• The check is strictly opt-in: unannotated structs are not touched. A malformed annotation, or one that doesn't resolve to an operation, is itself reported as an issue — the same failure mode as an unknown //meta:operation.
• The match-quality thresholds and the duplicated Go-initialisms list existed only to serve the fuzzy matcher and are removed with it.
• The Struct.Field exceptions file remains for annotated structs with a deliberate deviation, but its stale entries are dropped — under opt-in, a struct with known deviations simply isn't annotated until fixed.

Starter set

25 request types (26 annotations) are annotated in this PR — the dedicated request bodies already converted under #3644 (issue/gist/PR comments, milestones, keys, releases, merges, deployment branch policies, autolinks, custom repo roles, external groups, PR reviews, template repos). Against the current descriptions:

Found 0 schema field issues
Checked 26 annotations on 25 annotated structs

Future conversions can add their annotations incrementally, so coverage grows with #3644 instead of depending on a heuristic.

CI

check-schema-fields runs in the existing check-openapi job (it needs the downloaded descriptions and a GITHUB_TOKEN), alongside update-openapi --validate. The command exits non-zero on any issue.

Ref #4319.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.52%. Comparing base (0fb1d5e) to head (1fe00a7).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4375   +/-   ##
=======================================
  Coverage   98.52%   98.52%           
=======================================
  Files         195      195           
  Lines       17745    17745           
=======================================
  Hits        17484    17484           
  Misses        261      261           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gmlewis]

Thank you, @JamBalaya56562!
Overall, this is looking good, but there are many small helper functions in schema_fields.go that have no unit tests that I think could greatly benefit from some idiomatic Go table-driven unit tests to document their behavior.

Also, I would really like to see an example of the output of this tool (for example, when some of the exceptions are removed).

Finally, it would be nice if we didn't have to modify the code and that its exceptions could be updated by loading them from the config files like we do in the other tools.

[JamBalaya56562]

Thanks for the review, @gmlewis! I've addressed all three points in c6425df.

1. Unit tests — added idiomatic, table-driven tests for the helper functions in schema_fields.go (singularize, goName, isVersionToken, splitOpenAPIName, goNameCandidates, parseJSONTag, isPointerType/canBeOmitted, sharedFieldCount/hasEnoughSharedFields/sameJSONFieldSet, flattenObjectSchema/hasUnsupportedComposition, schemaProperties, canCheckOptionality, diagLocation, schemaFieldDiagnostic.String()), plus tests for the new exceptions loader.

2. Exceptions from config — moved schemaFieldExceptions out of Go source into tools/metadata/schema_field_exceptions.yaml, loaded at runtime (with a --exceptions flag to point at a different file). Adding/removing an exception no longer requires editing code; CONTRIBUTING.md is updated to match.

3. Example output — here is a run with the exceptions file emptied (--exceptions pointing at an empty file) so every currently-tolerated deviation surfaces. With the committed schema_field_exceptions.yaml the command reports Found 0 schema field issues.

$ script/metadata.sh check-schema-fields --exceptions empty.yaml --verbose
Found 7 schema field issues
Checked 4 OpenAPI schema/Go struct pairs; skipped 1138 OpenAPI schemas
github/dependency_graph_snapshots.go:83: DependencyGraphSnapshot.Detector (detector from snapshot): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/dependency_graph_snapshots.go:82: DependencyGraphSnapshot.Job (job from snapshot): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/dependency_graph_snapshots.go:81: DependencyGraphSnapshot.Ref (ref from snapshot): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/dependency_graph_snapshots.go:84: DependencyGraphSnapshot.Scanned (scanned from snapshot): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/dependency_graph_snapshots.go:80: DependencyGraphSnapshot.Sha (sha from snapshot): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/repos_deployment_branch_policies.go:29: DeploymentBranchPolicyRequest.Name (name from deployment-branch-policy-name-pattern-with-type): field is required and non-nullable in the OpenAPI schema but is a pointer [descriptions/api.github.com/api.github.com.json]
github/actions_workflow_runs.go:138: ReviewCustomDeploymentProtectionRuleRequest.Comment (comment from review-custom-gates-state-required): field is optional in the OpenAPI schema but is not a pointer, slice, map, interface, or selector type [descriptions/api.github.com/api.github.com.json]

(The three WorkflowsPermissionsOpt.* entries in the exceptions file don't currently match a schema by field set, so they produce no diagnostic today; I kept them from the original list, but I'm happy to drop them if you'd prefer the file to list only active deviations.)

[gmlewis]

Thank you, @JamBalaya56562 - please fix the new lint errors caused by the new unit tests, then we should be able to proceed with this PR.

[JamBalaya56562]

Thank you, @JamBalaya56562 - please fix the new lint errors caused by the new unit tests, then we should be able to proceed with this PR.

Thanks again, fixed.

[gmlewis]

Thank you, @JamBalaya56562!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.

cc: @stevehipwell - @alexandear - @Not-Dhananjay-Mishra

[stevehipwell]

@JamBalaya56562 I like the intent here but I'm not sure I'm onboard with the approach you've taken. Could you provide a list of the structs that are mapped and a list of the structs that are skipped? Ideally it'd be good to see how the mapped structs were mapped and if by name if the fields would have resulted in a match too?

What I'm currently thinking is that this linking is brittle and has the potential to cause churn for no real reason. IMHO it'd make more sense to use a //meta:schema comment on the struct to control the mapping and ignores as this would place the logic next to the code it's related to as well as making it deterministic.

[alexandear]

My biggest concern is that this command checks only 4 schema/Go structs, but we have many more.

Can we improve this?

[JamBalaya56562]

@stevehipwell thanks for the thorough review — I ran the numbers and I think you're right. Here is the full mapped/skipped breakdown you asked for (check-schema-fields --verbose).

Mapped (only 4 pairs, all via exact JSON field set — 0 by name):

deployment-branch-policy-name-pattern-with-type -> DeploymentBranchPolicyRequest
ghes-set-maintenance-request                    -> MaintenanceOptions
review-custom-gates-state-required              -> ReviewCustomDeploymentProtectionRuleRequest
snapshot                                        -> DependencyGraphSnapshot

To your question about name-vs-field cross-checking: nothing matched by name, so there is nothing to corroborate — the CI check relies entirely on exact field-set equality.

Skipped (1138):

reason	count
no unambiguous Go struct match	994
schema has no object properties (scalars/enums)	121
oneOf/anyOf/not	14
ambiguous field-set match (1 schema → several structs)	5
Go struct matches multiple schemas by field set	4

And the brittleness you flagged is concrete in that last bucket, e.g.:

AddProjectItemOptions matches repository-rule-params-actor / repository-rule-params-reviewer

An unrelated struct coincidentally matching rule-params schemas purely on {id, type}. The dropAmbiguousFieldSetMatches + schemaFieldExceptions I added are really just band-aids over this fragility, and @alexandear raised the same coverage concern in his review.

So I agree with your read: the exact-field-set heuristic gives very low recall (4 request structs) and real false positives — exactly the "brittle + churn" failure mode you described.

The field-comparison engine itself is sound (it did surface genuine required-vs-pointer mismatches on the structs it reached), so I'd like to keep that and replace only the matching: move to an explicit //meta:schema <schema-name> annotation on the struct (with an ignore form), mirroring the existing //meta:operation convention. That makes the mapping deterministic, keeps it next to the code, and lets us grow coverage intentionally instead of leaning on coincidental field-set equality.

I'll rework the PR along these lines. Since this started from @gmlewis's proposal in #4319, I want to make sure the annotation format works for all of you before I invest in the rewrite — happy to adjust the exact syntax.

[JamBalaya56562]

@alexandear thanks for the review — I pushed all nine inline fixes in aa993d88 (removed sliceSet, publicGithubClient, defaultJSONName, and the --exceptions/--json flags; slices.Clone; reflowed the added comments to the file's line-length convention; trimmed the exceptions-file header).

On your main concern — only 4 schema/struct pairs get checked — you and @stevehipwell are right, and that's the real limitation rather than the nits. The exact-field-set matching is inherently low-recall: most go-github request structs don't have a JSON field set identical to their OpenAPI schema, so the heuristic simply can't link them. I'd like to replace the matching with an explicit //meta:schema annotation on each request struct (keeping the field-comparison engine, which is the part that works), mirroring the existing //meta:operation convention. That's the change that actually grows coverage deterministically.

Since this started from @gmlewis's proposal in #4319, I'd like to confirm the annotation format with the three of you before reworking it — happy to hear preferences.

[JamBalaya56562]

@gmlewis Replying on the redesign direction (sorry for the wait). Following the coverage concern from @stevehipwell and @alexandear — 4/1142 schemas matched, with accidental-match risk — I'd like to replace the field-set matching entirely with explicit //meta:schema annotations, mirroring the existing //meta:operation convention:

// CreateCommitCommentRequest represents a request to create a commit comment.
//
//meta:schema request POST /repos/{owner}/{repo}/commits/{commit_sha}/comments
type CreateCommitCommentRequest struct { ... }

• request/response selects the operation's requestBody or 200-response schema (request bodies are usually inline/unnamed in the OpenAPI descriptions, so pointing at the operation rather than a component name works for both).
• The check becomes strictly opt-in: unannotated structs are skipped, annotated ones are validated field-by-field via json tags (no name matching, no heuristics), and an annotation that doesn't resolve to an operation is itself an error — same failure mode as //meta:operation.
• This dissolves both open threads: the match-quality threshold and the duplicated goInitialisms list only existed to serve the fuzzy matcher, and both go away with it.
• Coverage then grows explicitly: this PR would ship the machinery plus annotations for a starter set (e.g. the request types already converted under Refactor codebase to use value parameters instead of pointers where appropriate #3644), and further annotations land incrementally alongside future conversions.

If this direction works for you I'll rework this PR accordingly — or split the redesign into a fresh PR if you'd prefer this one closed. Either is fine on my side.

[gmlewis]

If this direction works for you I'll rework this PR accordingly — or split the redesign into a fresh PR if you'd prefer this one closed. Either is fine on my side.

That sounds like a good plan to me, and continuing in this PR is fine with me too. No rush.
You might want to update your branch from the latest master, though, as #4481 caused some churn.

[JamBalaya56562]

@gmlewis Done — the rework is pushed:

• 77ba61d merges the latest master (picking up the feat: Require Go 1.26 and use new(expr) for pointer literals #4481 churn and the v90 module path).
• ccd7712 replaces the fuzzy matching with the //meta:schema annotation resolution described above. The match thresholds and the duplicated initialisms list are gone with the matcher, so I've resolved those two threads; the stale exception entries are dropped too (under opt-in, a struct with known deviations simply isn't annotated until fixed).
• 1fe00a7 annotates a starter set of 25 request types (26 annotations) — the dedicated request bodies already converted under Refactor codebase to use value parameters instead of pointers where appropriate #3644. Against the current descriptions: Found 0 schema field issues / Checked 26 annotations on 25 annotated structs.

The PR description is updated to match. Local runs of the tools tests, the full ./github/ suite, and custom-gcl are green.

[gmlewis]

Ah, very nice, @JamBalaya56562!
My only concern is that when anyone makes a new PR that adds a struct, how will they know to opt-in?

Is there any way for the linter to give some kind of suggestion like "The following structs are missing the proposed metadata:" or something like that?

Or do we need a new tool that can fill in ALL missing struct metadata? Thoughts?

cc: @stevehipwell - @alexandear

[Not-Dhananjay-Mishra]

@JamBalaya56562 Since all API methods already have //meta:operation annotations, Is it not possible to get body/response structs from the method signature instead and compare those types against OpenAPI schema?

[JamBalaya56562]

Good question — that was effectively where the first iteration of this PR headed (it derived request structs from client.NewRequest body arguments), and it runs into three practical problems: (1) the signature type often isn't the wire body — several methods marshal an internal wrapper or an anonymous struct instead (CreateCommentInReplyTo, the Hook/Repository/Organization wrappers), (2) deriving from every method means checking the ~50 legacy pointer body types still on the paramcheck allowlist, which would light up CI with true-but-not-yet-fixable findings and need a huge exceptions list from day one, and (3) on the response side the shared all-pointer response types (*Repository etc.) serve dozens of operations with differing schemas. Explicit annotations keep the check opt-in so coverage grows as #3644 conversions land.

That said, signature-derived pairs could be a nice follow-up as a suggestion mode — e.g. listing unannotated candidates, or auto-inserting //meta:schema lines via update-go — without making them gate CI.