Skip to content

Conversation

@sirosen
Copy link
Contributor

@sirosen sirosen commented Oct 4, 2025

For the pull_request, pull_request_target, and push event types,
the schema was declared to both define properties and use a $ref
which defined properties (#/definitions/ref).

This usage is ambiguous at best, since it's unclear if the properties are
meant to be merged -- which is the intent and AJV's behavior -- or
overridden. These definitions are updated here to avoid using a $ref at
all, and greatly enhance the clarity of their semantics by nesting
oneOf and allOf in a way which better reflects the criteria being
declared.

The newly added negative test passes with AJV using the schema in
master but fails when run through python-jsonschema's toolchain.

Specifically, a failure is easy to produce with

pipx run check-jsonschema \
    --schemafile schemas/json/github-workflow.json \
    src/negative_test/github-workflow/bad_pull_request_event_declaration.yaml

With the schema updated to remove the unclear $ref loading, both tools
agree that the negative test input is incorrect.


I have also asked in the official JSON Schema community Slack to see if
anyone from the spec authorship group can confirm that this usage is
ambiguous or invalid.

It's possible that python-jsonschema is wrong, but I find it less likely
given AJV's existing reputation for going off-spec in a wide variety of cases.

Update: I got some more info from the JSON Schema spec maintainers.

In Draft 7 and earlier, the $ref [instructs] the implementation to ignore all of the other keywords in that schema object

So anywhere that you see a $ref used with other keywords, rather than the more typical "bare ref", in a Draft 7 schema, that means something is wrong.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 4, 2025

Thanks for the PR!

This section of the codebase is owned by @madskristensen and @hyperupcall - if they write a comment saying "LGTM" then it will be merged.

For the `pull_request`, `pull_request_target`, and `push` event types,
the schema was declared to both define properties and use a `$ref`
which defined properties (`#/definitions/ref`).

This usage is ambiguous at best, since it's unclear if the properties are
meant to be merged -- which is the intent and AJV's behavior -- or
overridden. These definitions are updated here to avoid using a $ref at
all, and greatly enhance the clarity of their semantics by nesting
`oneOf` and `allOf` in a way which better reflects the criteria being
declared.

The newly added negative test passes with AJV using the schema in
`master` but fails when run through `python-jsonschema`'s toolchain.

Specifically, a failure is easy to produce with

    pipx run check-jsonschema \
        --schemafile schemas/json/github-workflow.json \
        src/negative_test/github-workflow/bad_pull_request_event_declaration.yaml

With the schema updated to remove the unclear `$ref` loading, both tools
agree that the negative test input is incorrect.
@sirosen sirosen force-pushed the fix-github-workflows-schema-ref branch from b3365be to a830695 Compare October 4, 2025 02:49
@madskristensen madskristensen merged commit a75ed20 into SchemaStore:master Oct 6, 2025
4 checks passed
@madskristensen
Copy link
Contributor

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants