feat: more informative error message by reandreev · Pull Request #662 · zizmorcore/zizmor

reandreev · 2025-04-13T14:41:19Z

Addresses #646

This PR adds a workflow validation step after a failed unmarshaling attempt in order to find what the concrete issues with the workflows are. Running zizmor on the following workflow:

name: Invalid

boom:

on:
  workflow_call:
    inputs:
      input:
        description: Input

permissions: {}

jobs:
  valid:
    runs-on: ubuntu-latest
    steps:
      - run: echo 'invalid'

will produce the following output:

failed to register input: .github/workflows/invalid.yaml

Caused by:
    0: invalid GitHub Actions workflow: file://.github/workflows/invalid.yaml
       
       Caused by:
           on.workflow_call.inputs.input: "type" is a required property
           Additional properties are not allowed ('boom' was unexpected)
           
    1: invalid GitHub Actions definition: file://.github/workflows/invalid.yaml
       
       Caused by:
           missing field `runs`
    2: failed to register input as workflow or action

An interesting thing I've noticed is that running zizmor on the following workflow:

name: Invalid

boom:

on:
  workflow_call:

permissions: {}

jobs:
  valid:
    runs-on: ubuntu-latest
    steps:
      - run: echo 'invalid'

will succeed. Even though it has that additional boom property at the top level, zizmor still managed to unmarshal all of the expected properties correctly since those were properly defined and simply ignored the invalid one. Not sure if this is an expected behavior, but thought I'd mention it.

Regarding the validator instantiation, another approach would be to keep the GitHub Workflow schema as a dependency of the tool and instead do something like:

static WORKFLOW_VALIDATOR: LazyLock<Option<Validator>> = LazyLock::new(||
    match serde_json::from_str(include_str!("../github-workflow.json")) {
        Ok(schema) => validator_for(&schema).ok(),
        Err(_) => None,
    }
);

Considering how seldom the schema itself gets updated that could also be an approach to consider. Pros: the tool would be more self-contained, cons: the tool would need to be recompiled each time the schema gets updated.

Some other things to consider:

In the example above the error messages could be sorted by their depth/nesting and/or the order of appearance within the file itself
Line numbers could be added
The schema could be cached somewhere instead of retrieving it on every run

…attemp

woodruffw · 2025-04-13T17:15:45Z

Hi @reandreev, thanks for looking into this! I'll do a full review in the coming days.

Even though it has that additional boom property at the top level, zizmor still managed to unmarshal all of the expected properties correctly since those were properly defined and simply ignored the invalid one. Not sure if this is an expected behavior, but thought I'd mention it.

Yep, this is semi-unintentional behavior -- see zizmorcore/github-actions-models#33 for some context 🙂

Regarding the validator instantiation, another approach would be to keep the GitHub Workflow schema as a dependency of the tool and instead do something like:
static WORKFLOW_VALIDATOR: LazyLock<Option<Validator>> = LazyLock::new(||
    match serde_json::from_str(include_str!("../github-workflow.json")) {
        Ok(schema) => validator_for(&schema).ok(),
        Err(_) => None,
    }
); 

Yes, let's do this! IMO a network request in an error path like this isn't ideal; it'd be a lot faster and more reliable to bake the schema into the binary (and I agree that it won't change frequently).

woodruffw · 2025-04-17T03:35:18Z

No need to do it with this PR, but a thought: we'll probably want some kind of scheduled workflow that checks periodically for updates to this file.

(That concerns the CI though, so I'll do that in a followup.)

reandreev · 2025-04-25T16:44:28Z

+pub(crate) fn validate_action(contents: String) -> Error {
+    match serde_yaml::from_str(&contents) {
+        Ok(workflow) => match ACTION_VALIDATOR.apply(&workflow).basic() {
+            Valid(_) => anyhow!("valid action but failed unmarshaling"),


Not really sure how to handle this case since in theory this should be unreachable (?), maybe you have some suggestion on what to do or what error message to print out here?

unreachable makes sense to me here 🙂

Suggested change

Valid(_) => anyhow!("valid action but failed unmarshaling"),

Valid(_) => unreachable!("valid action but invalid YAML"),

This has the side effect of making this a panic, however, so the tests below should probably become integration snapshots instead -- see https://woodruffw.github.io/zizmor/development/#writing-snapshot-tests and https://github.com/woodruffw/zizmor/blob/ee6f160a304b96fbde2b6fe346e71218c1f07bf0/tests/integration/e2e.rs#L184-L195 for some docs/examples 🙂

I think maybe the tests can remain as unit tests by attempting to capture the panic instead, what do you think? For example:
https://github.com/woodruffw/zizmor/blob/f642eaf19a95534c42bf68169156eaf995a19261/src/utils.rs#L387-L389
Either way I did have to update a snapshot there so thanks for reminding me 🙂
Also, regarding your suggestion "valid action but invalid YAML", this unreachable snippet is only reached when there is a discrepancy between the schema validation and the unmarshaling logic (which in theory shouldn't happen), I'm not sure if the YAML is guaranteed to be invalid in those cases? 🤔

Also, regarding your suggestion "valid action but invalid YAML", this unreachable snippet is only reached when there is a discrepancy between the schema validation and the unmarshaling logic (which in theory shouldn't happen), I'm not sure if the YAML is guaranteed to be invalid in those cases? 🤔

Oh yeah, I see what you mean (and I misunderstood the context for this error) -- yeah, this should probably instead indicate to the user that the bug is on zizmor's modeling side, rather than an invalid workflow/action definition per the schema. Sorry for the confusion!

(This also undoes the need for the panic handling, so sorry for the wild goose chase there.)

woodruffw · 2025-04-30T04:39:20Z

Thanks @reandreev! I went ahead and landed this with #719, and credited you in the release notes.

reandreev added 2 commits April 13, 2025 15:19

feat: add workflow validation step after a failed workflow unmarshal …

3a6f891

…attemp

fix: formatting

6d2ac4c

reandreev added 2 commits April 13, 2025 19:22

add workflow schema

a392013

refactor validator to use embedded file

115d867

woodruffw reviewed Apr 17, 2025

View reviewed changes

Comment thread src/utils.rs Outdated

woodruffw reviewed Apr 17, 2025

View reviewed changes

Comment thread src/models.rs

woodruffw reviewed Apr 17, 2025

View reviewed changes

woodruffw added this to the 1.7.0 milestone Apr 17, 2025

woodruffw added the enhancement New feature or request label Apr 23, 2025

reandreev added 2 commits April 25, 2025 17:36

add github action schema

d13fae5

add github action validation

521ad4d

reandreev commented Apr 25, 2025

View reviewed changes

Comment thread src/utils.rs Outdated

reandreev and others added 5 commits April 27, 2025 17:37

change valid validation case to panic

6eff78b

update test snapshot

4f8e4ce

add expected panic

99e8a69

update panic message

f642eaf

Merge branch 'main' into main

991528b

woodruffw mentioned this pull request Apr 30, 2025

feat: more informative error message #719

Merged

woodruffw closed this Apr 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: more informative error message#662

feat: more informative error message#662
reandreev wants to merge 11 commits intozizmorcore:mainfrom
reandreev:main

reandreev commented Apr 13, 2025 •

edited

Loading

Uh oh!

woodruffw commented Apr 13, 2025

Uh oh!

Uh oh!

Uh oh!

woodruffw Apr 17, 2025

Uh oh!

reandreev Apr 25, 2025

Uh oh!

woodruffw Apr 27, 2025

Uh oh!

woodruffw Apr 27, 2025

Uh oh!

reandreev Apr 27, 2025

Uh oh!

woodruffw Apr 27, 2025

Uh oh!

Uh oh!

woodruffw commented Apr 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	Valid(_) => anyhow!("valid action but failed unmarshaling"),
	Valid(_) => unreachable!("valid action but invalid YAML"),

Uh oh!

Conversation

reandreev commented Apr 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

woodruffw commented Apr 13, 2025

Uh oh!

Uh oh!

Uh oh!

woodruffw Apr 17, 2025

Choose a reason for hiding this comment

Uh oh!

reandreev Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

woodruffw Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

woodruffw Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

reandreev Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

woodruffw Apr 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

woodruffw commented Apr 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

reandreev commented Apr 13, 2025 •

edited

Loading