Skip to content

fix: strip leading v from VERSION, guard unset SHELL in install.sh#47

Merged
min0625 merged 1 commit into
mainfrom
fix/version-strip-shell-unset
Jul 2, 2026
Merged

fix: strip leading v from VERSION, guard unset SHELL in install.sh#47
min0625 merged 1 commit into
mainfrom
fix/version-strip-shell-unset

Conversation

@min0625

@min0625 min0625 commented Jul 2, 2026

Copy link
Copy Markdown
Owner

Summary

  • Makefile: strip a leading v from VERSION so graft --version matches goreleaser's {{.Version}} for both local and released builds; add make cover-html; reorder check-tidy/cover targets.
  • script/install.sh: guard ${SHELL} with ${SHELL:-} so the script doesn't abort under set -u when SHELL is unset (e.g. minimal containers).

Test plan

  • CI (make check, tests) passes

…all.sh

VERSION now matches goreleaser's {{.Version}} so `graft --version` is
consistent between local builds and released binaries. install.sh no
longer aborts under `set -u` when SHELL is unset (e.g. minimal
containers). Also adds `make cover-html` and reorders check-tidy/cover
targets.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix VERSION normalization and harden install.sh for unset SHELL

🐞 Bug fix ✨ Enhancement ⚙️ Configuration changes 🕐 10-20 Minutes

Grey Divider

AI Description

• Strip leading "v" from Makefile VERSION to align local builds with goreleaser versions.
• Improve coverage tooling with coverpkg and a new cover-html target.
• Prevent install.sh from aborting under set -u when SHELL is unset.
Diagram

graph TD
  A[Developer/CI] --> B["Makefile"] --> C["VERSION normalized"] --> D["Binary version output"]
  B --> E["Coverage targets"] --> F["coverage.out / HTML"]
  G["install.sh"] --> H["PATH hint uses SHELL:-"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Normalize version string in Go code (runtime)
  • ➕ Keeps build tooling simple; normalization lives with version consumer
  • ➕ Works regardless of how VERSION is supplied (Makefile, CI, goreleaser)
  • ➖ Shifts release-policy logic into the binary
  • ➖ Still leaves build metadata inconsistent across tools/scripts
2. Standardize on tags without a leading 'v'
  • ➕ Eliminates need for normalization across tooling
  • ➕ Simplifies mental model: tag == version everywhere
  • ➖ Potentially disruptive change to existing tagging conventions
  • ➖ May require migration/updates in release automation and docs
3. Adjust goreleaser/template to include 'v' consistently
  • ➕ Keeps local VERSION unchanged if already 'v'-prefixed
  • ➕ Centralizes release naming policy in release tooling
  • ➖ Does not fix mismatch for local builds unless Makefile is also aligned
  • ➖ May affect downstream expectations of semantic version formatting

Recommendation: Current approach (strip leading 'v' in the Makefile and harden install.sh with ${SHELL:-}) is the most practical: it fixes the observed mismatch at the build-source and avoids introducing version-policy logic into the binary. The alternatives are viable if the project wants a different long-term versioning convention, but they are more disruptive than this targeted fix.

Files changed (2) +16 / -9

Bug fix (1) +4 / -3
install.shHandle unset SHELL under set -u when printing PATH hints +4/-3

Handle unset SHELL under set -u when printing PATH hints

• Updates shell detection to use ${SHELL:-} in case statements so the installer doesn't abort when SHELL is unset in minimal environments (e.g., containers). Keeps PATH hint behavior the same while making the script robust under nounset.

script/install.sh

Other (1) +12 / -6
MakefileNormalize VERSION and expand coverage targets +12/-6

Normalize VERSION and expand coverage targets

• Strips a leading 'v' from git tag-derived VERSION so local build version output matches goreleaser {{.Version}}. Improves coverage reporting by adding coverpkg=./... and introduces a cover-html target that generates coverage.html. Reorders check-tidy relative to coverage targets for clearer target grouping.

Makefile

@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 15 rules

Grey Divider


Remediation recommended

1. Broken source hint fallback 🐞 Bug ≡ Correctness
Description
With ${SHELL:-} the installer no longer aborts when SHELL is unset, but it can now emit a broken
instruction: source your shell config file (the placeholder contains spaces). This makes the PATH
hint copy/paste fail specifically in the minimal environments this change is targeting.
Code

script/install.sh[83]

+    case "${SHELL:-}" in
Evidence
The new ${SHELL:-} makes the fallback branch reachable when SHELL is unset; the function’s
fallback shell_config is a human placeholder and is later printed as an executable source
command, which becomes invalid when the placeholder contains spaces.

script/install.sh[76-96]
script/install.sh[109-118]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
After switching to `case "${SHELL:-}"`, the `SHELL`-unset path is no longer fatal, but the fallback `shell_config` value is a placeholder string with spaces. The script then prints `source ${shell_config}` without quoting/escaping, resulting in an invalid command users may copy/paste.

### Issue Context
This happens when `SHELL` is unset (common in minimal containers) or when it doesn’t match the known cases.

### Fix Focus Areas
- script/install.sh[76-120]

### Suggested change
Make the fallback hint unambiguous and copy/paste safe. Any of these would fix it:

1) Use a real default file (no spaces), e.g.:
```bash
*) shell_config='~/.profile' ;;
```

2) Or if you want to keep it as a placeholder, avoid printing a `source ...` line when `shell_config` is not a concrete path.

Optionally, also quote/escape the printed `source` command (bash supports `%q`) if you ever allow values that may contain spaces.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Extra sed in VERSION 🐞 Bug ⚙ Maintainability
Description
The Makefile now pipes VERSION through sed, adding an avoidable external dependency and extra
subprocess on every make invocation. This is unnecessary because the same “strip leading v”
behavior can be implemented using make-native string functions.
Code

Makefile[3]

+VERSION ?= $(shell (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD) | sed 's/^v//')
Evidence
The Makefile now shells out to sed to strip a leading v, even though the repo’s release pipeline
already stamps version without the v and make can do the same transformation internally.

Makefile[1-6]
.goreleaser.yaml[19-21]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`VERSION` is computed by spawning a shell pipeline that includes `sed` solely to strip a leading `v`. This adds an avoidable external dependency and process overhead.

### Issue Context
GoReleaser already stamps `main.version` with `{{.Version}}` (no leading `v`), so local builds should strip the `v` too, but this can be done without `sed`.

### Fix Focus Areas
- Makefile[1-6]

### Suggested change
Replace the `sed` usage with a make-native transformation, e.g.:

```make
VERSION ?= $(patsubst v%,%,$(shell git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD))
```

(or split into `VERSION_RAW` + `VERSION` if you want to keep it readable).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread Makefile
VERSION ?= $(shell git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD)
# Leading "v" is stripped to match goreleaser's {{.Version}}, so `graft --version`
# prints the same string for local builds and released binaries.
VERSION ?= $(shell (git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD) | sed 's/^v//')

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Informational

1. Extra sed in version 🐞 Bug ⚙ Maintainability

The Makefile now pipes VERSION through sed, adding an avoidable external dependency and extra
subprocess on every make invocation. This is unnecessary because the same “strip leading v”
behavior can be implemented using make-native string functions.
Agent Prompt
### Issue description
`VERSION` is computed by spawning a shell pipeline that includes `sed` solely to strip a leading `v`. This adds an avoidable external dependency and process overhead.

### Issue Context
GoReleaser already stamps `main.version` with `{{.Version}}` (no leading `v`), so local builds should strip the `v` too, but this can be done without `sed`.

### Fix Focus Areas
- Makefile[1-6]

### Suggested change
Replace the `sed` usage with a make-native transformation, e.g.:

```make
VERSION ?= $(patsubst v%,%,$(shell git describe --tags --exact-match 2>/dev/null || git rev-parse --short HEAD))
```

(or split into `VERSION_RAW` + `VERSION` if you want to keep it readable).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread script/install.sh
# environments (e.g. containers) where SHELL is unset.
# shellcheck disable=SC2088 # intentional: ~ is displayed as a hint to the user, not expanded
case "${SHELL}" in
case "${SHELL:-}" in

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Broken source hint fallback 🐞 Bug ≡ Correctness

With ${SHELL:-} the installer no longer aborts when SHELL is unset, but it can now emit a broken
instruction: source your shell config file (the placeholder contains spaces). This makes the PATH
hint copy/paste fail specifically in the minimal environments this change is targeting.
Agent Prompt
### Issue description
After switching to `case "${SHELL:-}"`, the `SHELL`-unset path is no longer fatal, but the fallback `shell_config` value is a placeholder string with spaces. The script then prints `source ${shell_config}` without quoting/escaping, resulting in an invalid command users may copy/paste.

### Issue Context
This happens when `SHELL` is unset (common in minimal containers) or when it doesn’t match the known cases.

### Fix Focus Areas
- script/install.sh[76-120]

### Suggested change
Make the fallback hint unambiguous and copy/paste safe. Any of these would fix it:

1) Use a real default file (no spaces), e.g.:
```bash
*) shell_config='~/.profile' ;;
```

2) Or if you want to keep it as a placeholder, avoid printing a `source ...` line when `shell_config` is not a concrete path.

Optionally, also quote/escape the printed `source` command (bash supports `%q`) if you ever allow values that may contain spaces.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@min0625 min0625 merged commit 78ed11b into main Jul 2, 2026
9 checks passed
@min0625 min0625 deleted the fix/version-strip-shell-unset branch July 2, 2026 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant