ojars/latvian-apostrofs

Watch

Fork 0

/simplify follow-up: release.yml + build-pkg.sh quality polish (bundle with #6 fix) #7

New issue

Closed

opened 2026-05-10 07:31:10 +03:00 by claude · 1 comment

claude commented

2026-05-10 07:31:10 +03:00

Collaborator

Origin

2026-05-10 session-end /simplify pass on the apostrofs PR #4 + #5 diff surfaced four small quality findings in release.yml and build-pkg.sh. None are critical (no real bugs), all are inside files that latvian-apostrofs#6 will rewrite when the GITHUB_TOKEN scope issue is resolved. Bundling here so whoever picks up #6 fixes them in the same touch instead of a separate roundtrip.

Findings

`release.yml`

set -euo pipefail only on the Create Forgejo release step (line 46). The Resolve version from tag, Build PKG, Compute SHA256, and Print Homebrew Cask bump command steps lack it. Forgejo Actions defaults bash to -e for run: blocks but not -u or pipefail. Concrete failure mode: PKG_SHA=$(shasum -a 256 "${PKG}" | cut -d' ' -f1) would silently produce empty output if shasum errored (cut still exits 0). Fix: add set -euo pipefail to every run: block.
Redundant SHA256 computation (lines 29-36). build-pkg.sh already prints SHA256: <hash> at line 163; the separate Compute SHA256 step re-runs shasum -a 256 over the entire PKG just to capture into $GITHUB_OUTPUT. Sub-second on a small PKG, but it's a true no-op duplication. Fix: have build-pkg.sh write to $GITHUB_OUTPUT directly when GITHUB_OUTPUT is set: [[ -n "${GITHUB_OUTPUT:-}" ]] && echo "pkg_sha=${PKG_SHA}" >> "$GITHUB_OUTPUT". Then drop the separate step. Net: one less shasum pass, one less workflow step.
Heredoc delimiter is fragile to a one-character edit (lines 103-116). cat <<MANUAL_STEP is unquoted, so ${VERSION} and ${PKG_SHA} expand correctly. If a future maintainer ever quotes the delimiter as <<'MANUAL_STEP', the literal ${VERSION} text will print instead. Not broken, just brittle. Cosmetic — defer or skip.

`build-pkg.sh`

pkgutil --check-signature on unsigned PKG is guaranteed dead code (line ~161). PKG has no --sign flag. pkgutil --check-signature always exits non-zero on unsigned content; the 2>&1 | head -5 || true swallows everything. Either remove the line entirely (no signing planned), or replace with # TODO(#NN): sign with Developer ID — see <ticket> and drop the || true so the verify step actually fails when signing breaks.
VERSION="${VERSION:-0.0.0-dev}" fallback creates poison pkgutil receipts (lines 15+28). If a developer runs bash build-pkg.sh locally without VERSION= set, the produced PKG installs at version 0.0.0-dev and registers lv.kapteinis.latvian-apostrofs at that version system-wide. A subsequent brew install --cask apostrofs may then trip Cask's "already installed by other means" detection. Fix: refuse to build without explicit VERSION= unless ALLOW_DEV_VERSION=1 is set, OR change the dev fallback string to something that won't cause confusion in pkgutil --pkg-info output.

Acceptance

When #6 is closed, the touched-anyway diff should also pick up findings 1, 2, 4, 5. Finding 3 is cosmetic — defer.

Won't-fix from /simplify

Cross-language safety-check duplication (bash uninstall.sh + Ruby apostrofs.rb uninstall_preflight) — correct as-is per agent review; both files are <40 lines, two distribution paths, identical magic string + comment.
Unanchored 'Latvian (apostrofs)' grep/include match — theoretical false-match only; tightening adds future-fragility against macOS defaults format changes.

## Origin 2026-05-10 session-end `/simplify` pass on the apostrofs PR #4 + #5 diff surfaced four small quality findings in `release.yml` and `build-pkg.sh`. None are critical (no real bugs), all are inside files that `latvian-apostrofs#6` will rewrite when the GITHUB_TOKEN scope issue is resolved. Bundling here so whoever picks up #6 fixes them in the same touch instead of a separate roundtrip. ## Findings ### `release.yml` 1. **`set -euo pipefail` only on the `Create Forgejo release` step** (line 46). The `Resolve version from tag`, `Build PKG`, `Compute SHA256`, and `Print Homebrew Cask bump command` steps lack it. Forgejo Actions defaults `bash` to `-e` for `run:` blocks but **not** `-u` or `pipefail`. Concrete failure mode: `PKG_SHA=$(shasum -a 256 "${PKG}" | cut -d' ' -f1)` would silently produce empty output if `shasum` errored (cut still exits 0). Fix: add `set -euo pipefail` to every `run:` block. 2. **Redundant SHA256 computation** (lines 29-36). `build-pkg.sh` already prints `SHA256: <hash>` at line 163; the separate `Compute SHA256` step re-runs `shasum -a 256` over the entire PKG just to capture into `$GITHUB_OUTPUT`. Sub-second on a small PKG, but it's a true no-op duplication. Fix: have `build-pkg.sh` write to `$GITHUB_OUTPUT` directly when `GITHUB_OUTPUT` is set: `[[ -n "${GITHUB_OUTPUT:-}" ]] && echo "pkg_sha=${PKG_SHA}" >> "$GITHUB_OUTPUT"`. Then drop the separate step. Net: one less shasum pass, one less workflow step. 3. **Heredoc delimiter is fragile to a one-character edit** (lines 103-116). `cat <<MANUAL_STEP` is unquoted, so `${VERSION}` and `${PKG_SHA}` expand correctly. If a future maintainer ever quotes the delimiter as `<<'MANUAL_STEP'`, the literal `${VERSION}` text will print instead. Not broken, just brittle. Cosmetic — defer or skip. ### `build-pkg.sh` 4. **`pkgutil --check-signature` on unsigned PKG is guaranteed dead code** (line ~161). PKG has no `--sign` flag. `pkgutil --check-signature` always exits non-zero on unsigned content; the `2>&1 | head -5 || true` swallows everything. Either remove the line entirely (no signing planned), or replace with `# TODO(#NN): sign with Developer ID — see <ticket>` and drop the `|| true` so the verify step actually fails when signing breaks. 5. **`VERSION="${VERSION:-0.0.0-dev}"` fallback creates poison pkgutil receipts** (lines 15+28). If a developer runs `bash build-pkg.sh` locally without `VERSION=` set, the produced PKG installs at version `0.0.0-dev` and registers `lv.kapteinis.latvian-apostrofs` at that version system-wide. A subsequent `brew install --cask apostrofs` may then trip Cask's "already installed by other means" detection. Fix: refuse to build without explicit `VERSION=` unless `ALLOW_DEV_VERSION=1` is set, OR change the dev fallback string to something that won't cause confusion in `pkgutil --pkg-info` output. ## Acceptance When #6 is closed, the touched-anyway diff should also pick up findings 1, 2, 4, 5. Finding 3 is cosmetic — defer. ## Won't-fix from /simplify - Cross-language safety-check duplication (bash `uninstall.sh` + Ruby `apostrofs.rb` `uninstall_preflight`) — correct as-is per agent review; both files are <40 lines, two distribution paths, identical magic string + comment. - Unanchored `'Latvian (apostrofs)'` grep/include match — theoretical false-match only; tightening adds future-fragility against macOS `defaults` format changes.

ojars referenced this issue from a commit

2026-05-10 13:21:32 +03:00

release.yml + build-pkg.sh: /simplify polish bundle (latvian-apostrofs#7)

claude commented

2026-05-10 13:23:17 +03:00

Author

Collaborator

Closed — fixed end-to-end on 2026-05-10

Findings 1, 2, 4, 5 applied (finding 3 deferred per ticket as cosmetic). Validated end-to-end via v2026.05.2 tag-push:

Workflow run task 2886 succeeded with the new merged-hash flow (one less workflow step, one less shasum pass).
Release v2026.05.2 created with PKG asset; release body's SHA matches shasum -a 256 of the downloaded asset (a7ac7ba0dd107f3df58eecb7b8dc53269490c29b66cf401b82c634cd2a267f29).
Cask bumped to 2026.05.2 (homebrew-pareizrakstiba@1664657).

Local-dev refusal verified:

$ unset VERSION && bash build-pkg.sh
Error: VERSION env var not set.
       CI sets VERSION from the git tag (release.yml step 'Build PKG installer').
       For a local dev build: ALLOW_DEV_VERSION=1 bash build-pkg.sh
$ echo $?
1

Escape hatch verified: ALLOW_DEV_VERSION=1 bash build-pkg.sh proceeds with Version: 0.0.0-dev.

Commit: 5a52fbe (release.yml + build-pkg.sh, +21/-12).

Finding 3 (heredoc delimiter cosmetic) deferred per the original ticket's acceptance — current cat <<MANUAL_STEP (unquoted) is correct for the variable expansion that step requires; quoting the delimiter would silently break it, but that's a future-foot-gun, not a current bug. No action.

## Closed — fixed end-to-end on 2026-05-10 Findings 1, 2, 4, 5 applied (finding 3 deferred per ticket as cosmetic). Validated end-to-end via `v2026.05.2` tag-push: - Workflow run [task 2886](https://git.kapteinis.lv/ojars/latvian-apostrofs/actions/runs/2886) succeeded with the new merged-hash flow (one less workflow step, one less shasum pass). - Release [v2026.05.2](https://git.kapteinis.lv/ojars/latvian-apostrofs/releases/tag/v2026.05.2) created with PKG asset; release body's SHA matches `shasum -a 256` of the downloaded asset (`a7ac7ba0dd107f3df58eecb7b8dc53269490c29b66cf401b82c634cd2a267f29`). - Cask bumped to 2026.05.2 (`homebrew-pareizrakstiba@1664657`). **Local-dev refusal verified:** ``` $ unset VERSION && bash build-pkg.sh Error: VERSION env var not set. CI sets VERSION from the git tag (release.yml step 'Build PKG installer'). For a local dev build: ALLOW_DEV_VERSION=1 bash build-pkg.sh $ echo $? 1 ``` **Escape hatch verified:** `ALLOW_DEV_VERSION=1 bash build-pkg.sh` proceeds with `Version: 0.0.0-dev`. **Commit:** `5a52fbe` (release.yml + build-pkg.sh, +21/-12). **Finding 3 (heredoc delimiter cosmetic) deferred** per the original ticket's acceptance — current `cat <<MANUAL_STEP` (unquoted) is correct for the variable expansion that step requires; quoting the delimiter would silently break it, but that's a future-foot-gun, not a current bug. No action.

claude closed this issue

2026-05-10 13:23:17 +03:00

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

ojars/latvian-apostrofs#7

No description provided.

Rows
Columns