verify-action-build: add node_modules verification for vendored deps#652
verify-action-build: add node_modules verification for vendored deps#652
Conversation
| ) | ||
| ) | ||
| all_match = True | ||
| else: |
There was a problem hiding this comment.
ok, then for consistency this should also be a else if has_node_modules:, I guess?
There was a problem hiding this comment.
No, not really - the if above is for non-js actions. (docker/composite). JS actions can have either:
- node_modules + (index.js /main.js)
- dist (main.js / index.js)
There was a problem hiding this comment.
the current code calls both diff_js_files and diff_node_modules if has_node_modules, but it should call only diff_node_modules, right?
to clarify, I'm not concerned about performance or 'purity' or anything here, but I do think it's helpful to keep the code as easy to understand as possible - otherwise it'll get harder to evolve (even with help from agents) and trust its output.
There was a problem hiding this comment.
Since the prior if block is long maybe add a comment then to describe what falls through into the else. Maybe put each portion into functions?
There was a problem hiding this comment.
I will refactor it to make it easier to understand and reason and add tests after I maerge that one. Would that be ok ?
There was a problem hiding this comment.
My plan is to split it into smaller better reusable methods - and have clear logic outside of those methods and almost not have conditiona logic inside the methods.
That should make it much easier to reason about.
Some GitHub Actions ship source JS directly with committed node_modules/ instead of bundling into dist/ (e.g. leafo/gh-actions-luarocks). Previously the script detected this pattern but skipped verification entirely. Now when committed node_modules/ is detected, the script saves the original, does a fresh production install, and compares the two trees by SHA256 hash. Package.json install metadata fields are filtered to avoid false positives. Generated-by: Claude
potiuk
force-pushed
the
verify-node-modules
branch
from
f01fc86 to
2b030cb
Compare
|
I will wait with resolving and approving the above - before refactoring |
3 participants
Summary
node_modules/for actions that ship source JS directly instead of bundling intodist/(e.g.leafo/gh-actions-luarocks)node_modules/, does a freshnpm install --production(or yarn/pnpm equivalent), and compares the two trees by SHA256 hash_resolved,_integrity, etc.) are filtered to avoid false positivesTest plan
leafo/gh-actions-luarocks@97053c556d6ce2c8e26eb7ac93743437c7af7248— 269 files in 6 packages all matchactions/checkout(dist-based action) — existing flow unaffectedGenerated with Claude Code