action-validator icon indicating copy to clipboard operation
action-validator copied to clipboard

feat: Glob validation for Node/WASM bindings

Open bcheidemann opened this issue 9 months ago • 1 comments

This PR implements glob validation for the Node/WASM bindings. Currently this is achieved using the existing mechanism for accessing system functions instead of one of the approaches described in https://github.com/mpalmer/action-validator/issues/40.

bcheidemann avatar Mar 03 '25 15:03 bcheidemann

Since this is branched off from https://github.com/mpalmer/action-validator/pull/86, I've marked it as a draft until this is merged. I'll then rebase this PR and mark it as being ready for review.

bcheidemann avatar Mar 03 '25 15:03 bcheidemann

@mpalmer now that https://github.com/mpalmer/action-validator/pull/86 has landed, I've rebased this PR and it's now ready for review

bcheidemann avatar Oct 09 '25 13:10 bcheidemann

The PR itself looks reasonable. Given the number of "globbing doesn't work exactly the same as GitHub's" issues that have been filed, I can't help but wonder: how similar are the matching semantics of the JS fast-glob package, compared to the glob crate? My concern is that we'll end up with a bunch more "globbing doesn't work right" issues, but they'll be native/WASM-specific issues.

To clarify, I'm not against merging this as-is, I guess I just want to know what to expect in the issue tracker once this lands.

mpalmer avatar Oct 22 '25 22:10 mpalmer

Given the number of "globbing doesn't work exactly the same as GitHub's" issues that have been filed, I can't help but wonder: how similar are the matching semantics of the JS fast-glob package, compared to the glob crate?

Good question!

I have added some tests based on GitHub's documentation for their "filter patterns" (see https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#filter-pattern-cheat-sheet).

As far as I can tell, the behavior is the same for fast-glob as for the glob crate, except that fast-glob supports the extended gitignore syntax (e.g. *.{js,jsx,ts,tsx}) while glob does not. This would actually a bug in the Node version of AV since GitHub doesn't support the extended syntax. I'd be happy to look into a fix for this when I have some time. Would you consider this a blocker for the current PR or something which can be fixed in a followup?

As it stands, it seems most of the GitHub syntax is supported by both AV native and node, which I've asserted with the addition of this test.

Currently supported:

  • "*" ✅
  • "**" ✅
  • "?" ✅
  • "+" 🚫
  • "[]" ✅
  • "!" ✅

I've also added a test for rejecting extended gitignore syntax, but this is currently ignored when running npm test because of the aforementioned bug.

bcheidemann avatar Oct 23 '25 10:10 bcheidemann

Would you consider this a blocker for the current PR or something which can be fixed in a followup?

No, our globbing support is already at odds with GitHub's; "differently broken" isn't a blocker, I just like to know what to say when the next round of "this globbing corner case doesn't work" issues come in.

mpalmer avatar Oct 28 '25 23:10 mpalmer