-
Notifications
You must be signed in to change notification settings - Fork 3
fix: reject when command fails. perform commit with --no-verify #40
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughReplaced a custom Promise wrapper with Node's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| } | ||
|
|
||
| await execPromise(`git commit -m "chore(release): v${nextVersion}"`) | ||
| await execPromise(`git commit --no-verify -m "chore(release): v${nextVersion}"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not keen on this - I think if there is a pre-commit that fails, something weird is going on and we shouldn't proceed with making a new version until it's fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh... this is only commiting a readme and the package.json so the precommit isn't all that relevant imo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This came out of a case where husky was complaining about something in a repository and caused it to fail. All I was doing was making a release after a PR or two had been merged, so fixing husky was more effort than it was worth.
I don't mind too much if we don't have this no-verify, as it will now fail rather than continue through to creating the tag, and I could manually complete the steps. But I also don't see any harm in including it here, especially as the expected behaviour is that the lint-staged will conclude that all the staged files are ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @bin/updateVersion.mjs:
- Around line 8-10: promisify(exec) returns an object { stdout, stderr } not a
string, so update all call sites of execPromise to use the stdout string before
applying string methods: either destructure const { stdout } = await
execPromise(cmd) or replace (await execPromise(cmd)) with (await
execPromise(cmd)).stdout; apply this to uses around execPromise where rawDiff is
produced, where rawDiff.split('+++') is called, and where gitHash and
commitDateStr are trimmed so you call .trim() on their stdout-derived strings
(e.g., gitHash = (await execPromise(...)).stdout.trim()).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
bin/updateVersion.mjs
🔇 Additional comments (1)
bin/updateVersion.mjs (1)
186-186: LGTM:--no-verifyflag appropriately bypasses hooks for automated commits.This correctly addresses the issue where husky misconfiguration could cause commits to fail silently, resulting in tags being created without corresponding commits.
| import { promisify } from 'node:util' | ||
|
|
||
| const execPromise = promisify(exec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical: promisify(exec) returns { stdout, stderr }, not a string.
The previous custom wrapper likely returned just the stdout string. With promisify(exec), the result is an object. All existing usages that call string methods (.split(), .trim()) on the result will fail at runtime.
Affected lines:
- Line 55:
(await execPromise(...)).split('\n') - Line 59-60:
rawDiff.split('+++') - Line 98, 101:
gitHash.trim() - Line 99-100:
commitDateStr.trim()
🐛 Proposed fix: access `.stdout` property on all command results
-const tags = cli.flags.lastTag || (await execPromise('git tag -l --sort=-v:refname')).split('\n')
+const tags = cli.flags.lastTag || (await execPromise('git tag -l --sort=-v:refname')).stdout.split('\n')-const rawDiff = await execPromise(`git log --format=+++%s__%b__%h__%H ${lastTag}..HEAD`)
+const rawDiff = (await execPromise(`git log --format=+++%s__%b__%h__%H ${lastTag}..HEAD`)).stdout- const gitHash = await execPromise(`git rev-parse --short HEAD`)
- const commitDateStr = await execPromise(`git log -1 --pretty=format:%ct HEAD`)
+ const gitHash = (await execPromise(`git rev-parse --short HEAD`)).stdout
+ const commitDateStr = (await execPromise(`git log -1 --pretty=format:%ct HEAD`)).stdout🤖 Prompt for AI Agents
In @bin/updateVersion.mjs around lines 8 - 10, promisify(exec) returns an object
{ stdout, stderr } not a string, so update all call sites of execPromise to use
the stdout string before applying string methods: either destructure const {
stdout } = await execPromise(cmd) or replace (await execPromise(cmd)) with
(await execPromise(cmd)).stdout; apply this to uses around execPromise where
rawDiff is produced, where rawDiff.split('+++') is called, and where gitHash and
commitDateStr are trimmed so you call .trim() on their stdout-derived strings
(e.g., gitHash = (await execPromise(...)).stdout.trim()).
About the Contributor
This pull request is posted on behalf of myself.
Type of Contribution
This is a: Bug fix
Current Behavior
While making a release of a library, I was confused that it didnt make a commit. I then discovered it had made a tag, based on the existing commit.
This is because husky is misconfigured and is causing the commit to fail.
New Behavior
This ensures that when a commaand fails, it will cause a rejection and result in an error.
Skip running any commit hooks when making the commit, as they shouldn't matter and cause unexpected failures in this simple command
Testing Instructions
Other Information
Status
This PR fixes two critical issues in the version update automation script: