Release hygiene: checksums, test gate, no PDBs, correct ICU/macOS/uninstall guidance

[BrettKinny]

Summary

Stacked on #169 (fix/third-party-notices) — that PR already modifies release.yml and the README License section. Review/merge it first; this will retarget to main once it merges.

Release/install/docs hygiene from the v1.0 follow-up review (docs/V1-REVIEW-FOLLOWUP.md, items 13, 16 + the release lows):

• ICU guidance removed: the published binary has InvariantGlobalization=true and never loads ICU, but install.sh warned fresh-system users and the README told Ubuntu 24.04/Debian 13 users to install libicu72 (Debian 12's package). Both removed; CONTRIBUTING.md untouched — developers running the test projects genuinely need ICU.
• SHA256 checksums: the release job now publishes a SHA256SUMS asset covering every archive, and install.sh/install.ps1 verify the downloaded archive against it — hard fail on mismatch, graceful warning when SHA256SUMS doesn't exist (older releases) or no hash tool is available.
• No more .pdb in release archives: publish runs with -p:DebugType=none.
• Test gate on releases: a new test job (full dotnet test -c Release on ubuntu) gates the build matrix — previously a tag on an untested commit published untested binaries.
• Fork PRs no longer get a red X from claude-code-review.yml (job-level fork guard; the secret isn't available to forks).
• uninstall.sh now prompts to remove ~/.local/share/opcilloscope (PKI/cert stores), matching uninstall.ps1 and the README's manual instructions — it previously declared success while leaving certificate data behind.
• Docs: CLAUDE.md's CLI section gains the missing --insecure flag; README notes that macOS binaries are unsigned (Gatekeeper quarantine + xattr -d com.apple.quarantine / curl-installer workaround). .gitignore gains publish/.

Test plan

• All workflow YAMLs parse; bash -n clean on install.sh/uninstall.sh; checksum match/tamper paths simulated functionally
• dotnet build Opcilloscope.sln -c Release — 0 warnings, 0 errors
• pwsh unavailable in the build environment — install.ps1 changes follow the file's existing patterns but want a Windows smoke test
• Next tagged release: verify SHA256SUMS asset, no .pdb in archives, test job gates the matrix

---

Part of the v1 follow-up punch list (docs/V1-REVIEW-FOLLOWUP.md, items 13, 16 + lows).

https://claude.ai/code/session_012Vopnd9vWkzELveHRgZhie

---

Generated by Claude Code

[BrettKinny]

Note on the red claude-review check: it's expected on this PR and can be ignored. The action's token exchange refuses to run when the PR modifies claude-code-review.yml itself ("The workflow file must exist and have identical content to the version on the repository's default branch") — and this PR adds the fork guard to that file. The action's own error text says to ignore it in this case; the check will run normally once this merges.

---

Generated by Claude Code

[claude]

Code Review — PR #174: Release hygiene

Note: This PR is already merged. Feedback is provided for posterity and to inform future work.

Overview

Solid release-hygiene PR that closes several real gaps: SHA-256 integrity verification for downloaded binaries, a test gate before release builds, stripping PDB symbols from archives, correct ICU/macOS install guidance, and a long-overdue uninstall fix for certificate stores. All changes are purposeful and well-scoped.

---

release.yml

Strengths

• New test job on ubuntu-latest with fetch-depth: 0 (needed by MinVer) correctly gates the build matrix. Previously a tag on an untested commit could publish broken binaries.
• -p:DebugType=none strips symbols from release archives — good for size and preventing unintentional source-path leakage.
• SHA256SUMS generation (sha256sum * in the collected assets directory) and uploading it alongside the archives is the right pattern.

Minor observations

• The test job only runs on ubuntu-latest. The integration tests use an in-process OPC UA server, so platform parity is not critical here, but if Windows-only code paths are ever added this could silently miss them. Low risk for now.
• fetch-depth: 0 in the test job fetches the entire history just to run tests. MinVer won't fail a build-less dotnet test, so this is harmless but slightly wasteful.

---

install.sh — verify_checksum()

Strengths

• Clean, well-parameterized function with three distinct graceful-degradation paths (no SHA256SUMS file, no matching entry, no hash tool). Correct use of command -v sha256sum / shasum -a 256 fallback for macOS.
• error call on mismatch correctly aborts (assuming error() calls exit 1, which it does in the existing script).

Potential issue

The grep pattern uses a single leading space before the filename. sha256sum emits two spaces between hash and filename in binary mode (the default). This still works because the pattern matches any line ending with filename regardless of preceding spaces — but it is fragile. If the SHA256SUMS format ever changes, expected would silently be empty, causing verification to be skipped rather than failing loudly. A more defensive pattern would use grep -E "[[:space:]]${archive_name}$" or match two spaces explicitly.

---

install.ps1

Potential bug — checksum abort path

if ($actual -ne $expected) {
    Write-Err "Checksum mismatch for ${archiveName}. ... Aborting."
}
Write-Info "Checksum verified (SHA-256)."

If Write-Err is a display-only wrapper (like Write-Host with color) rather than an exit-calling wrapper, execution falls through to Write-Info "Checksum verified" and continues installing a potentially tampered archive. On a security-critical tamper-detection path, the abort should be explicit with an exit 1 after Write-Err. Worth verifying and patching even if Write-Err does call exit 1 internally — defense in depth here matters.

---

uninstall.sh

Strengths

• [ -t 0 ] to detect non-interactive stdin is exactly the right idiom here. Defaulting to "N" when piped (safe default) and prompting when interactive is a clean UX pattern.
• The comment explaining why stdin may already be consumed when piped from curl is a helpful breadcrumb for future maintainers.

Minor nit
The removed_something flag is not updated when the data directory is removed. The final message ("Opcilloscope has been uninstalled." vs "Nothing to uninstall.") could be misleading if only certificates were removed and the binary was already absent. A removed_something=true in the cert-removal branch would be consistent with how the rest of the function tracks state.

---

claude-code-review.yml

The job-level if: github.event.pull_request.head.repo.fork == false guard is the correct pattern for workflows that consume secrets unavailable to forks. Comment is clear and accurate.

---

README / CLAUDE.md / .gitignore

• macOS Gatekeeper note with the xattr -d com.apple.quarantine one-liner is accurate and helpful.
• ICU section removal is correct — InvariantGlobalization=true means no libicu dependency at runtime.
• --insecure flag added to CLAUDE.md CLI docs — it was previously missing.
• publish/ in .gitignore is necessary given the -o ./publish output path used by the release workflow.

---

Summary

Area	Status
Test gate on release	Solid
SHA256SUMS generation	Correct
install.sh checksum verification	Works; minor grep fragility noted
install.ps1 checksum abort path	WARN: Verify Write-Err exits; add explicit exit 1 as defence
uninstall.sh data-dir prompt	Well implemented
PDB stripping	Good
Fork guard	Good
Docs / README / CLAUDE.md	Good

The install.ps1 abort path is the one item worth a follow-up patch. Everything else is clean.