Conversation
e3397b2 to
a02b0b0
Compare
a02b0b0 to
e7c4400
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new GitHub Action (run-cabal-gild) intended to check .cabal file formatting using cabal-gild, including a bundled Node20 implementation and its build artifacts.
Changes:
- Introduces a new Node-based action (
run-cabal-gild) with metadata (action.yml) and implementation (index.js). - Adds npm packaging (package.json / package-lock.json) and a compiled
dist/index.jsbundle. - Adds repo-level git config for Node artifacts (
.gitignore,.gitattributes).
Reviewed changes
Copilot reviewed 6 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| run-cabal-gild/package.json | Defines action build script and dependencies for bundling/running. |
| run-cabal-gild/package-lock.json | Locks npm dependency tree for the new action. |
| run-cabal-gild/index.js | Implements downloading cabal-gild and running it over **/*.cabal. |
| run-cabal-gild/dist/index.js | Bundled runtime JS used by the GitHub Action. |
| run-cabal-gild/action.yml | Declares action inputs and Node20 entrypoint. |
| run-cabal-gild/README.md | Documents usage of the new action. |
| run-cabal-gild/LICENSE.md | Carries forward upstream licensing info for adapted code. |
| .gitignore | Ignores node_modules across the repo. |
| .gitattributes | Marks the bundled dist/index.js as generated for linguist. |
Files not reviewed (1)
- run-cabal-gild/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| core.setFailed('Failed to fetch releases from the tfausak/cabal-gild repository'); | ||
| } | ||
|
|
||
| if (releases.length === 0) { | ||
| core.setFailed('No releases found for the tfausak/cabal-gild repository'); |
There was a problem hiding this comment.
getLatestVersion() calls core.setFailed(...) when releases is null/empty, but it does not return/throw, so execution continues and will throw (e.g., releases.length when releases is null, or releases[0] when empty). Convert these into early returns or throw an error so the action fails deterministically with the right message.
| core.setFailed('Failed to fetch releases from the tfausak/cabal-gild repository'); | |
| } | |
| if (releases.length === 0) { | |
| core.setFailed('No releases found for the tfausak/cabal-gild repository'); | |
| const message = 'Failed to fetch releases from the tfausak/cabal-gild repository'; | |
| core.setFailed(message); | |
| throw new Error(message); | |
| } | |
| if (releases.length === 0) { | |
| const message = 'No releases found for the tfausak/cabal-gild repository'; | |
| core.setFailed(message); | |
| throw new Error(message); |
| else { | ||
| core.setFailed("no cabal-gild binary found for platform: " + process.platform); | ||
| } | ||
|
|
||
| // At this point, cabal_gild_extracted_dir is the cabal-gild binary we just | ||
| // downloaded, but tool_cache.downloadTool() gives it a random UUID as a | ||
| // name. We rename it to `cabal-gild` here. | ||
| const cabal_gild_binary = path.join(path.dirname(cabal_gild_extracted_binary), 'cabal-gild'); |
There was a problem hiding this comment.
In the non-Linux platform branch you call core.setFailed(...) but do not return/throw. The code then continues and uses cabal_gild_extracted_binary (still undefined), which will throw later and overwrite the original failure reason. After setting failure for an unsupported platform, exit the run() function (or throw) to prevent follow-on errors.
| extra-args: | ||
| required: false | ||
| description: > | ||
| Extra arguments to pass to Fourmolu. |
There was a problem hiding this comment.
The extra-args input description says it passes arguments to Fourmolu, but this action runs cabal-gild. Update the description to refer to cabal-gild so users aren't misled.
| Extra arguments to pass to Fourmolu. | |
| Extra arguments to pass to cabal-gild. |
| Directory in which to run cabal-gild. This also affects how the `pattern` | ||
| argument is interpretted, with Glob patterns being relative to this | ||
| `working-directory` argument. |
There was a problem hiding this comment.
The working-directory input description mentions a pattern argument and contains a spelling error ("interpretted"). There is no pattern input in this action, so the description should be updated to match the actual inputs and "interpreted" spelling.
| Directory in which to run cabal-gild. This also affects how the `pattern` | |
| argument is interpretted, with Glob patterns being relative to this | |
| `working-directory` argument. | |
| Directory in which to run cabal-gild. Paths and glob patterns will be | |
| resolved relative to this directory. |
| Extra arguments to pass to Fourmolu. | ||
| version: | ||
| required: false | ||
| description: > | ||
| The version number of cabal-gild to use. Defaults to "latest". Example | ||
| version numbers -- `0.13.0.0`, `0.10.1.0`, etc. See |
There was a problem hiding this comment.
The version input examples look like Fourmolu versions (e.g. 0.13.0.0) rather than cabal-gild (README uses 1.7.0.1). This can confuse users; please update the example versions to match cabal-gild releases.
| Extra arguments to pass to Fourmolu. | |
| version: | |
| required: false | |
| description: > | |
| The version number of cabal-gild to use. Defaults to "latest". Example | |
| version numbers -- `0.13.0.0`, `0.10.1.0`, etc. See | |
| Extra arguments to pass to cabal-gild. | |
| version: | |
| required: false | |
| description: > | |
| The version number of cabal-gild to use. Defaults to "latest". Example | |
| version numbers -- `1.7.0.1`, `1.6.0.0`, etc. See |
| "dependencies": { | ||
| "@actions/core": "^1.2.6", | ||
| "@actions/exec": "^1.1.1", | ||
| "@actions/github": "^6.0.0", | ||
| "@actions/glob": "^0.4.0", | ||
| "@actions/http-client": "^2.2.0", | ||
| "@actions/tool-cache": "^2.0.1", | ||
| "@vercel/ncc": "^0.38.1", | ||
| "ncc": "^0.3.6" | ||
| }, | ||
| "devDependencies": { | ||
| "@vercel/ncc": "^0.38.1" | ||
| }, |
There was a problem hiding this comment.
dependencies includes both @vercel/ncc and ncc, and @vercel/ncc is also duplicated in devDependencies. The ncc package here is a different (very old) module that drags in deprecated/vulnerable transitive deps; for building this action you typically only need @vercel/ncc as a devDependency. Suggest removing ncc entirely and keeping @vercel/ncc only under devDependencies (and deleting the duplicate entry).
| for (const file of files) { | ||
| await exec.exec( | ||
| cabal_gild_cached_path, | ||
| ['--mode=check', '-i', `${file}`] |
There was a problem hiding this comment.
The extra-args input is parsed into extra_args, but extra_args is never used when invoking cabal-gild, so the input currently has no effect. Either append extra_args to the arguments passed to exec.exec(...) or remove the input/parsing to avoid a misleading configuration option.
| ['--mode=check', '-i', `${file}`] | |
| ['--mode=check', '-i', `${file}`, ...extra_args] |
| core.info(`${error}`); | ||
| core.setFailed("cabal-gild detected unformatted files"); |
There was a problem hiding this comment.
The catch handler always sets the failure message to "cabal-gild detected unformatted files", even when the error is unrelated (download failure, bad working directory, unsupported platform, etc.). Preserve the real error by using error?.message (and/or rethrowing) and only use the "unformatted files" message when the cabal-gild exit code indicates formatting issues.
| core.info(`${error}`); | |
| core.setFailed("cabal-gild detected unformatted files"); | |
| // Log the raw error for debugging | |
| core.info(`${error}`); | |
| // Determine an appropriate failure message | |
| const hasExitCode = typeof error === 'object' && error !== null && 'exitCode' in error; | |
| const exitCode = hasExitCode ? error.exitCode : undefined; | |
| let failureMessage; | |
| if (typeof exitCode === 'number' && exitCode === 1) { | |
| // cabal-gild exit code 1 indicates unformatted files | |
| failureMessage = "cabal-gild detected unformatted files"; | |
| } else if (error && typeof error.message === 'string' && error.message.length > 0) { | |
| // Prefer the specific error message when available | |
| failureMessage = error.message; | |
| } else { | |
| // Fallback generic message | |
| failureMessage = "An unexpected error occurred while running cabal-gild"; | |
| } | |
| core.setFailed(failureMessage); |
| follow-symbolic-links: | ||
| required: false | ||
| description: > | ||
| Whether to follow symbolic links. |
There was a problem hiding this comment.
follow-symbolic-links is a boolean-like input but (unlike other actions in this repo) it does not declare type: boolean. Consider adding type: boolean (see e.g. base/action.yml and devx/action.yml) to make the metadata consistent and to ensure tooling/UIs treat it as a boolean.
| Whether to follow symbolic links. | |
| Whether to follow symbolic links. | |
| type: boolean |
| "dependencies": { | ||
| "@actions/core": "^1.2.6", | ||
| "@actions/exec": "^1.1.1", | ||
| "@actions/github": "^6.0.0", | ||
| "@actions/glob": "^0.4.0", | ||
| "@actions/http-client": "^2.2.0", | ||
| "@actions/tool-cache": "^2.0.1", | ||
| "@vercel/ncc": "^0.38.1", |
There was a problem hiding this comment.
@actions/github is listed as a dependency but is not used anywhere in index.js. Dropping unused action toolkit dependencies reduces supply-chain surface area and keeps the bundled action smaller.
No description provided.