Conversation
Member
Author
|
Split into many commits to hopefully simplify review, PTAL. I'm no expert in any of this, so I'm relying quite a bit on Fable. |
Bump @actions/core, @actions/exec, and @actions/tool-cache to their latest CommonJS majors; the newest majors are ESM-only and would require migrating the action and its tests to ESM. Bump the dev dependencies to their latest versions, with two caveats: jest 30 removed the toBeCalled matcher aliases, and TypeScript stays on 5.x since ts-jest caps it below 7.0 and 6.0 force-enables esModuleInterop, which breaks the spy-based tests. Deny the unrs-resolver postinstall script, which npm blocks with a warning and jest works fine without. Rebuild dist accordingly. The jest tests pass again with these versions. Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
cue commands now require a language version in module.cue; add it via 'cue mod fix'. The genworkflows tasks generated inside the comprehension no longer have their dependency on the repoRoot exec task tracked, failing with a non-concrete stdout. Drop the indirection and write the files relative to the working directory, which is always the repo root when running cue cmd. Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Replace the schemas vendored from schemastore.org under cue.mod/pkg with the cue.dev/x/githubactions module from the Central Registry, which CUE resolves and fetches automatically. This makes the vendorgithubschema command and its CI step redundant. Regenerating action.yml with CUE v0.17.1 also reorders some fields. Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
Use the latest actions/checkout and actions/setup-node with node 24, matching the action's new runtime, and test against CUE v0.17.1, the current latest release. Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
The tests were disabled in CI when they broke with newer dependencies; they pass again, so run them in the step that so far only ran 'npm ci'. Importing src/run.ts invoked run() as a side effect, so jest ran it unmocked and core.setFailed marked the process as failed even with all tests passing. Only invoke run() when executed as the entrypoint, and rebuild dist. Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
|
👋 |
Member
Author
|
Thanks for the ping - I forgot to follow up here. |
myitcv
approved these changes
Aug 27, 2026
myitcv
left a comment
myitcv
left a comment
Member
There was a problem hiding this comment.
LGTM, thanks. I reviewed this commit by commit and verified the claims hands-on in a local checkout:
npm ci && npm run distreproducesdist/index.jsbyte-identical, so the committed bundle is a faithful build ofsrc/with the locked deps.- All 14 jest tests pass, and they also pass with
npm ci --ignore-scripts, confirming that denying theunrs-resolverpostinstall viaallowScriptsis safe. - The
require.main === moduleguard survives the ncc bundling:require('./dist/index.js')does not invokerun(), whilenode dist/index.jsdoes (failing on the missingversioninput as expected). cue cmd genactionandcue cmd genworkflowswith CUE v0.17.1 (matching CI) regenerate everything with zero diff.actions/checkout@v7andactions/setup-node@v7exist, v0.17.1 is the latest CUE release, and@actions/core2.0.3 /@actions/exec2.0.0 /@actions/tool-cache3.0.1 are the latest majors before the ESM-only ones, as the commit message says.
A few optional, non-blocking suggestions:
cue.dev/x/githubactionshas moved on to v0.8.0 since this PR was authored. I checked thatcue mod get cue.dev/x/githubactions@v0.8.0followed by both regen commands produces no diff beyondcue.mod/module.cue, so bumping is free if you want it fresh at merge time.npm auditflagsundici@6.27.0(moderate advisories), which ships indist/index.jsvia@actions/core->@actions/http-client@3.0.2(^6.23.0). undici 6.28.0 fixes it and is in range, sonpm audit fixplus adistrebuild would clear it. The other audit hits (brace-expansion,js-yaml) are jest-only dev deps and do not ship.- Pre-existing and out of scope here, but worth a follow-up:
action.ymldeclares the output ascue-path, while the code sets (and the README documents)cuectl-path, so the declared output name is never actually set.
One question on "all: fix CUE tooling for CUE v0.17": is the tools/flow regression (comprehension-generated tasks losing their dependency on the repoRoot exec task) filed upstream? I could not find an issue for it, and it would be good to reference one rather than only working around it.
(Review assisted by Claude Code, claude-fable-5.)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(see commit messages - please do not squash)