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

[JamBalaya56562]

As proposed by @gmlewis in #4319, this adds a check-schema-fields subcommand to the metadata tool that compares the JSON field optionality of Go request structs against GitHub's OpenAPI schemas, so that "required-but-pointer" (and similar) mismatches — which @stevehipwell noted are tedious to verify by hand — are caught automatically.

What it does

For each mutating request-body struct it matches the struct to its OpenAPI component schema (by exact JSON field set, or by schema name in --schema mode) and reports fields whose optionality disagrees with the schema:

• a required, non-nullable field that is a pointer,
• a required field that has omitempty/omitzero,
• an optional field that is not omittable (not a pointer/slice/map/interface).

Only request-body structs are checked by default: a struct is treated as a request body when it is passed as the body argument to a mutating client.NewRequest call and is not also returned as a response (*T/[]*T), so response and shared all-pointer types are not flagged. --include-responses and --schema opt into broader checks.

Exceptions

Following the same convention as paramcheck/structfield, a small schemaFieldExceptions list in tools/metadata/schema_fields.go records Struct.Field deviations that are intentionally tolerated for now (each a known item to fix and remove over time). This keeps the check green while letting the codebase be cleaned up incrementally.

CI

Wired into script/lint.sh / .github/workflows/linter.yml under CHECK_GITHUB_OPENAPI, alongside the existing update-openapi --validate check. Documented in CONTRIBUTING.md.

Ref #4319

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.51%. Comparing base (da81e17) to head (aa993d8).
⚠️ Report is 4 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4375   +/-   ##
=======================================
  Coverage   97.51%   97.51%           
=======================================
  Files         193      193           
  Lines       19526    19526           
=======================================
  Hits        19040    19040           
+ Misses        269      268    -1     
- Partials      217      218    +1     

☔ 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.

[gmlewis]

This function seems to have some magic numbers and calculations in it that need a comment as to how they were chosen.

[JamBalaya56562]

Good call. I pulled the numbers into named constants and documented them:

const (
    minSharedFieldsForMatch = 3  // accept once this many JSON fields overlap
    minSharedFieldPercent   = 60 // else the overlap must cover this share of the smaller field set
)

Rationale (now in a comment): a schema-name match already agrees on the Go type name, so this guard only rejects a name that coincidentally collides with an unrelated struct. It's deliberately lenient — a wrong match is dropped later as ambiguous, but a missed match silently skips a real check. So tiny types (≤2 fields) must share every field, while larger types match once 3 fields overlap or the overlap covers ≥60% of the smaller field set (compared as shared*100 >= smallest*minSharedFieldPercent to stay in integer math).

[gmlewis]

It looks like we now will have two sets of "Go initialisms" that we will have to keep track of.
Could they possibly be shared to reduce the maintenance burden?

[JamBalaya56562]

The other set lives in the tools/structfield module (initialisms / specialCases), which is a separate Go module from tools/metadata with no shared package, and it's keyed/shaped differently (an uppercase-keyed set used for a different job). Sharing them would mean introducing a new shared module that both tools depend on.

For now I've kept this list small — it only needs the initialisms that actually appear in OpenAPI schema names — and added a comment noting the duplication is intentional. Happy to do the shared-module extraction as a follow-up if you'd prefer; let me know.

[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.