Conversation
…ke test (#513) The two release-safety shell tests only ran when someone remembered to invoke them by hand. release.sh is exercised only during a release — on main, under time pressure, with a tag about to be pushed — so a regression there surfaces at the worst possible moment. Four consecutive releases hit exactly that. A test that runs on every PR moves the discovery forward. Wire both into a dedicated pure-bash `Release Script Tests` job: - test_release_changelog.sh — [Unreleased] promotion (added #512, covers #511) - test_release_client_pin.sh — stale/unpublished client-pin guard (added #516) Both source release.sh with RELEASE_SH_LIB=1, run in a temp dir, and need no Go build or network, so the job skips Go setup and stays ~1s. Delete test_cli_smoke.sh instead of wiring it: it builds ./cmd/lvt, which was extracted to github.com/livetemplate/lvt (commit 2e595d2), so the script errors immediately and has no callers anywhere. Generator smoke coverage now lives in that repo's e2e suite — this drops no protection. Removing it also resolves the issue's shebang-portability note; it was the only test script still using #!/bin/bash. The repo enforces no required status checks (ruleset carries only deletion + non_fast_forward), so this job is a visible signal the maintainer heeds at merge, like every existing check. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ui2cwpeGkrUfRt8rh2FgGG
ReviewOverviewWires the two existing (but never-CI-run) release-safety bash test scripts — Correctness
Style/Conventions
Risks / Things to double check
Test coverage
VerdictSmall, low-risk CI change that does exactly what the description says. No functional issues found. |
✅ Performance Benchmark ResultsStatus: No significant regressions detected Benchmark ComparisonThresholds
See benchmarking guide for details. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #513.
What
scripts/had shell test scripts that never ran in CI — they only executed when someone remembered to invoke them by hand, so the things they protect could regress silently. This wires the release-safety tests into CI and removes a dead one.Wire both release tests into a dedicated
Release Script Testsjobtest_release_changelog.sh—[Unreleased]promotion logic (added in fix(release): promote the curated [Unreleased] instead of regenerating #512, covers the release.sh discards the curated [Unreleased] section on every successful release #511 defect)test_release_client_pin.sh— the stale/unpublished client-pin guard (added in fix(release): refuse to release on a stale client pin #516, same unwired gap)Both source
release.shwithRELEASE_SH_LIB=1, run in a temp dir, and need no Go build or network, so the job skips Go setup entirely and runs in ~1s.release.shis only exercised during a release — onmain, under time pressure, with a tag about to be pushed — which is the worst possible moment to discover a regression, and exactly the situation the last four releases produced. Running these on every PR moves that discovery forward.Delete
test_cli_smoke.shinstead of wiring itThe issue named it as a script to wire in, but its premise is stale: it builds
./cmd/lvt, which was extracted togithub.com/livetemplate/lvt(commit2e595d29). The script errors immediately (directory not found) and has zero callers anywhere in the tree. Generator smoke coverage now lives in that repo's e2e suite (github.com/livetemplate/lvt/e2e/), so this deletion drops no protection — it just removes a stale local copy.Removing it also resolves the issue's shebang-portability note: it was the only test script still using
#!/bin/bash(which doesn't exist on NixOS and some distros). The remaining scripts already use#!/usr/bin/env bash, and the job invokes them viabash scripts/...regardless.Note on gating
The repo enforces no required status checks (its ruleset carries only
deletion+non_fast_forward), so this job — like every existing check — is a visible signal the maintainer heeds at merge time rather than a hard gate. Making it block would be a one-line ruleset addition if you want that.Not in scope
client/scripts/test_release_changelog.sh(fix(release): promote the curated [Unreleased] instead of regenerating client#154) is a separate repo.TestTopic_V8_…CrossInstanceand itsawaitWSContainshelper class) panics under concurrentgo test ./...Docker load. It's onmainalready and untouched by this diff; it's being handled as its own root-cause investigation.Verification
bash scripts/test_release_changelog.sh→ all checks passbash scripts/test_release_client_pin.sh→ all checks passunit-tests,fuzz-tests,release-script-tests)go test ./...suite green via the pre-commit hook🤖 Generated with Claude Code