feat: add cut-version script for versioning#1651
feat: add cut-version script for versioning#1651Alxandr wants to merge 1 commit intowe-promise:mainfrom
Conversation
brin-security-scanner
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 30 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 80 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
📝 Walkthrough🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Alxandr
force-pushed
the
feat/cut-version-script
branch
from
c61108e to
8e76252
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c61108e172
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| line_count="$(wc -l < .sure-version)" | ||
| if [[ "${line_count}" -ne 1 ]]; then | ||
| log::error ".sure-version must contain exactly one line" | ||
| return 1 |
There was a problem hiding this comment.
The validation in check::sure-version uses wc -l to enforce a single-line file, but wc -l counts newline characters rather than logical lines. A valid file containing just 1.2.3 (no trailing \n) is treated as 0 lines and rejected, which makes the release script fail on otherwise valid version files created by common editors/automation. Consider validating by reading the file content directly (and checking for additional lines) instead of relying on newline count.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The idea is that cut-release should own the version-file, so this is a valid fault-state.
brin-security-scanner
Bot
added
the
pr:flagged
Brin PR Security ScanThis PR has findings that should be reviewed.
Findings:
Analyzed by Brin |
To be fair, this is a valid point. I just based it on how the repo said to do it. It might be better to just copy the 1 file from the source repository in (vendor it) to reduce this risk. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
scripts/cut-version (1)
460-475: No rollback on mid-flight failure — document recovery steps.With
set -Eeuo pipefail, any step failure (e.g. network error duringpush-tag, or asedissue inupdate-version) exits immediately. If the tag was already pushed but the version-file commit was not, the repo is left with a pushed tag pointing at the un-bumped commit. Consider documenting the manual recovery path (delete + re-push the tag, or re-run with--no-tag) inVERSIONING.mdor the script's error messages.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cut-version` around lines 460 - 475, The run::cut-version function can leave a pushed tag without a corresponding version commit if a later step (step::push-tag, step::update-version, step::commit, step::push) fails; add a documented recovery path and a hint in the script: update VERSIONING.md with explicit manual recovery steps (delete the remote tag and re-create it or re-run with a --no-tag/force option) and add a brief on-error message in run::cut-version that prints those steps and suggests re-running with --no-tag or deleting the tag (e.g., "git push --delete origin vX.Y.Z" then re-run) so users know how to rollback after mid-flight failures.scripts/cut-version.test.sh (1)
119-124: ⚡ Quick winMissing
assert_failtests for the three forbidden prerelease transitions.The guarded transitions (
alpha→beta,alpha→rc,beta→alpha,beta→rc,rc→*) are the most error-prone paths in the version logic. The current suite only covers the happy path.bash_unitprovidesassert_failfor exactly this purpose:➕ Suggested additions
+test::version::next-alpha::from-beta-fails() { + assert_fail "version::next-alpha '1.2.3-beta.1'" +} + +test::version::next-alpha::from-rc-fails() { + assert_fail "version::next-alpha '1.2.3-rc.1'" +} + +test::version::next-beta::from-rc-fails() { + assert_fail "version::next-beta '1.2.3-rc.1'" +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/cut-version.test.sh` around lines 119 - 124, Add negative tests using bash_unit's assert_fail to cover the forbidden prerelease transitions that currently lack coverage: create tests alongside test::version::next-alpha::from-alpha-increments-alpha (and equivalent test suites for next-beta/next-rc) that call the version functions (e.g., version::next-alpha, version::next-beta, version::next-rc) with inputs representing the guarded transitions (alpha→beta, alpha→rc, beta→alpha, beta→rc, and any rc→* attempts) and assert_fail that the function returns non-zero; ensure each forbidden input is its own test case and uses assert_fail instead of assert_equals so the suite verifies these transitions are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 88-90: Replace the unpinned, silent piped installer by fetching a
specific, pinned release (tag or commit SHA) and failing on HTTP errors: update
the curl invocation (the current "curl -s
https://raw.githubusercontent.com/.../install.sh | bash") to download a release
tarball or a pinned install.sh URL and verify integrity (e.g., checksum or GPG)
before executing, and enable curl failure visibility (use -f and -S or -fsSL) so
CI fails on HTTP errors; do not pipe arbitrary upstream content to bash—save the
downloaded file, verify it, then run it or prefer installing via the official
release artifact instead of piping from master.
In `@scripts/cut-version`:
- Around line 107-111: The current line_count="$(wc -l < .sure-version)"
miscounts files missing a trailing newline; change the line counting to a method
that counts logical lines regardless of trailing newline (e.g., use grep -c ''
or an awk/counting approach) when computing line_count for .sure-version, then
keep the existing check that calls log::error ".sure-version must contain
exactly one line" and returns 1; ensure the replacement still captures failures
(use || true or similar) so the script doesn't exit unexpectedly and preserves
the existing variable name line_count and error path.
- Around line 411-412: The sed invocations in scripts/cut-version use `sed -i -E
...` which on BSD sed creates backup files like Chart.yaml-E; change these two
invocations that target charts/sure/Chart.yaml to use a portable in-place edit
pattern (e.g., `-i.bak` with the -E flag) and then remove the generated `.bak`
files so no backup remains in the working tree; update both instances that
perform the version and appVersion substitutions accordingly and ensure the
cleanup (rm) runs after both edits.
---
Nitpick comments:
In `@scripts/cut-version`:
- Around line 460-475: The run::cut-version function can leave a pushed tag
without a corresponding version commit if a later step (step::push-tag,
step::update-version, step::commit, step::push) fails; add a documented recovery
path and a hint in the script: update VERSIONING.md with explicit manual
recovery steps (delete the remote tag and re-create it or re-run with a
--no-tag/force option) and add a brief on-error message in run::cut-version that
prints those steps and suggests re-running with --no-tag or deleting the tag
(e.g., "git push --delete origin vX.Y.Z" then re-run) so users know how to
rollback after mid-flight failures.
In `@scripts/cut-version.test.sh`:
- Around line 119-124: Add negative tests using bash_unit's assert_fail to cover
the forbidden prerelease transitions that currently lack coverage: create tests
alongside test::version::next-alpha::from-alpha-increments-alpha (and equivalent
test suites for next-beta/next-rc) that call the version functions (e.g.,
version::next-alpha, version::next-beta, version::next-rc) with inputs
representing the guarded transitions (alpha→beta, alpha→rc, beta→alpha, beta→rc,
and any rc→* attempts) and assert_fail that the function returns non-zero;
ensure each forbidden input is its own test case and uses assert_fail instead of
assert_equals so the suite verifies these transitions are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ad59f48-1dcf-4fda-982f-12952d23a2b6
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/publish.yml.gitignoreVERSIONING.mdscripts/cut-versionscripts/cut-version.test.sh
💤 Files with no reviewable changes (1)
- .github/workflows/publish.yml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
89-92:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the
bash_unitinstaller before executing it in CI.This is the same CI supply-chain risk flagged earlier:
curl -s .../master/install.sh | bashcan execute unreviewed upstream changes and will also hide download failures. Please pin the installer to a release tag or commit SHA and fail fast on HTTP errors.🔧 Suggested change
- curl -s https://raw.githubusercontent.com/bash-unit/bash_unit/master/install.sh | bash + curl -fsSL https://raw.githubusercontent.com/bash-unit/bash_unit/<tag-or-sha>/install.sh -o /tmp/bash_unit_install.sh + bash /tmp/bash_unit_install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 89 - 92, The CI step that installs bash_unit via curl -s https://.../master/install.sh | bash is unsafe; update the "Test shell scripts" job to fetch a pinned installer (use a specific release tag or commit SHA in the URL instead of master), use curl -fSL to fail-fast on HTTP errors, download the script to a file first (do not pipe directly to bash), optionally verify a known checksum or GPG signature, then run the downloaded script before running FORCE_COLOR=true ./bash_unit ./scripts/*.test.sh; ensure the command sequence around the installer URL and the test invocation (the install step and the FORCE_COLOR test runner) is updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 89-92: The CI step that installs bash_unit via curl -s
https://.../master/install.sh | bash is unsafe; update the "Test shell scripts"
job to fetch a pinned installer (use a specific release tag or commit SHA in the
URL instead of master), use curl -fSL to fail-fast on HTTP errors, download the
script to a file first (do not pipe directly to bash), optionally verify a known
checksum or GPG signature, then run the downloaded script before running
FORCE_COLOR=true ./bash_unit ./scripts/*.test.sh; ensure the command sequence
around the installer URL and the test invocation (the install step and the
FORCE_COLOR test runner) is updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9381e1ee-ef60-46e8-b7e3-92abdac87dfd
📒 Files selected for processing (6)
.github/workflows/ci.yml.github/workflows/publish.yml.gitignoreVERSIONING.mdscripts/cut-versionscripts/cut-version.test.sh
💤 Files with no reviewable changes (1)
- .github/workflows/publish.yml
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- VERSIONING.md
- scripts/cut-version.test.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/cut-version
Also adds a VERSIONING.md document explaiing the versioning scheme.
Alxandr
force-pushed
the
feat/cut-version-script
branch
from
8e76252 to
9e54782
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vendor/scripts/bash_unit`:
- Around line 159-176: The custom error prefix variable message in
assert_within_delta is used in the precondition asserts before it is set;
initialize message early (e.g., declare local message and assign it from $4 or
to empty string) before the three assert calls so those asserts include the
caller-provided prefix; update the assert_within_delta function to set local
message=${4:-} (or local message="" then message=${4:-}) prior to calling
is_number/assert so the custom message is preserved when validation fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4f52dd8d-0059-406e-8383-326b10898f04
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/publish.yml.gitignoreVERSIONING.mdscripts/cut-versionscripts/cut-version.test.shvendor/scripts/bash_unit
💤 Files with no reviewable changes (1)
- .github/workflows/publish.yml
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- scripts/cut-version.test.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- VERSIONING.md
- scripts/cut-version
| assert_within_delta() { | ||
| function abs() { | ||
| local value=$1 | ||
| local sign=$(( value < 0 ? -1 : 1 )) | ||
| echo $((value * sign)) | ||
| } | ||
| function is_number() { | ||
| local value=$1 | ||
| test "$value" -eq "$value" 2>/dev/null | ||
| } | ||
| local expected=$1 | ||
| local actual=$2 | ||
| local max_delta=$3 | ||
| assert "is_number $expected" "$message expected value [$expected] is not a number" | ||
| assert "is_number $actual" "$message actual value [$actual] is not a number" | ||
| assert "is_number $max_delta" "$message max_delta [$max_delta] is not a number" | ||
| local message=${4:-} | ||
| [[ -z $message ]] || message="$message\n" |
There was a problem hiding this comment.
Initialize message before the validation asserts.
The custom message is read on Lines 172-174 before it is assigned on Line 175, so callers lose their prefix whenever one of the numeric precondition checks fails.
💡 Proposed fix
assert_within_delta() {
+ local expected=$1
+ local actual=$2
+ local max_delta=$3
+ local message=${4:-}
+ [[ -z $message ]] || message="$message\n"
+
function abs() {
local value=$1
local sign=$(( value < 0 ? -1 : 1 ))
echo $((value * sign))
}
function is_number() {
local value=$1
test "$value" -eq "$value" 2>/dev/null
}
- local expected=$1
- local actual=$2
- local max_delta=$3
assert "is_number $expected" "$message expected value [$expected] is not a number"
assert "is_number $actual" "$message actual value [$actual] is not a number"
assert "is_number $max_delta" "$message max_delta [$max_delta] is not a number"
- local message=${4:-}
- [[ -z $message ]] || message="$message\n"
local actual_delta="$(abs $((expected - actual)))"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert_within_delta() { | |
| function abs() { | |
| local value=$1 | |
| local sign=$(( value < 0 ? -1 : 1 )) | |
| echo $((value * sign)) | |
| } | |
| function is_number() { | |
| local value=$1 | |
| test "$value" -eq "$value" 2>/dev/null | |
| } | |
| local expected=$1 | |
| local actual=$2 | |
| local max_delta=$3 | |
| assert "is_number $expected" "$message expected value [$expected] is not a number" | |
| assert "is_number $actual" "$message actual value [$actual] is not a number" | |
| assert "is_number $max_delta" "$message max_delta [$max_delta] is not a number" | |
| local message=${4:-} | |
| [[ -z $message ]] || message="$message\n" | |
| assert_within_delta() { | |
| local expected=$1 | |
| local actual=$2 | |
| local max_delta=$3 | |
| local message=${4:-} | |
| [[ -z $message ]] || message="$message\n" | |
| function abs() { | |
| local value=$1 | |
| local sign=$(( value < 0 ? -1 : 1 )) | |
| echo $((value * sign)) | |
| } | |
| function is_number() { | |
| local value=$1 | |
| test "$value" -eq "$value" 2>/dev/null | |
| } | |
| assert "is_number $expected" "$message expected value [$expected] is not a number" | |
| assert "is_number $actual" "$message actual value [$actual] is not a number" | |
| assert "is_number $max_delta" "$message max_delta [$max_delta] is not a number" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@vendor/scripts/bash_unit` around lines 159 - 176, The custom error prefix
variable message in assert_within_delta is used in the precondition asserts
before it is set; initialize message early (e.g., declare local message and
assign it from $4 or to empty string) before the three assert calls so those
asserts include the caller-provided prefix; update the assert_within_delta
function to set local message=${4:-} (or local message="" then message=${4:-})
prior to calling is_number/assert so the custom message is preserved when
validation fails.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
jjmata
removed
the
contributor:flagged
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Looks good on first (quick) inspection ... let me get through PR backlog a bit and come back to this. 🙏
2 participants
Also adds a VERSIONING.md document explaiing the versioning scheme.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores