ci: exercise the release deb checks on PRs, and fix what running them found - #317
Merged
Conversation
`release.yml`'s packaged-artifact smoke and dependency-closure check were ~110 lines of inline bash that only fire on a `v*` tag, in a job that `needs: create-release` — so they first execute *after* the GitHub Release is already published, and had never run in Actions at all. Extracting them lets CI run the same code on ordinary PRs (next commit). Three scripts, all mode 755: `smoke-deb.sh` (payload + control metadata + the real point, that the packaged binary's compiled-in default binds the production `roost` IPC namespace), `verify-deb-closure.sh` (install and launch in a clean `--no-install-recommends` container, where nothing is preinstalled), and `resolve-one-deb.sh`. The third exists because a `<deb-path>` contract cannot host the "exactly one deb" assertion, and leaving that in the callers would mean the release steps still contain logic; all three call sites now share it. Behaviour is preserved, with four deliberate widenings: the payload check covers all seven `packaging/nfpm.yaml` destinations rather than the two binaries; `dpkg-deb -f` asserts Package/Architecture and, when given, Version — which makes nfpm's `-`→`~` normalization checked instead of assumed; `ROOST_BUNDLE_PROFILE` is explicitly unset (merely never setting it is not the same thing when run by hand); and each script owns its XDG sandbox, which is load-bearing rather than hygiene — in CI these run after steps that already launched a UI, so without it the smoke could get a successful `identify` from an earlier instance and pass while proving nothing. Running it found a defect that had been sitting unexecuted on main: the closure check HUNG for 31 minutes in a shed and had to be killed, and its failure message asserted "its Depends: list is incomplete" for what was actually a hang — a misdiagnosis that would have sent someone hunting a dependency bug that did not exist. Both are release-path defects whose only backstop was the job's 90-minute timeout, burned after the Release is public. Now: `timeout` on both the docker run and the in-container launch, an explicit reap of the backgrounded UI, and a message that separates timeout (harness/environment) from non-zero exit. The timeout also puts xvfb-run in its own process group, which is what stops the background UI holding the container's streams open — 31 minutes became 8.6 seconds. Shed-verified on arm64 against a real .deb, with negative controls so a green run means something: a featureless build fails on the namespace assertion, a stripped `Depends:` fails on `libXcursor.so.1`, a dropped packaging destination fails on the payload, two debs fail resolution, and bad arguments are rejected. Evidence in the plan's artifact folder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
The release path's only real artifact checks — does the packaged binary bind the production IPC namespace, and is its Depends: closure complete — first executed during an actual release, after `create-release` had already published the GitHub Release. Now the same scripts run on any PR that can change what the .deb contains. Gated on a new, deliberately narrow `deb` filter rather than the existing `linux` one. `linux` folds in the *rustcore anchor, so it is true on essentially every Rust PR; gating on it would have paid an nfpm download and a Docker Hub pull per PR. `release.yml` is in the new filter because it appears in no other filter at all — a PR editing only the release workflow previously triggered nothing capable of validating it — and `Cargo.lock` is there because a new dependency can change the runtime closure. No new job, so `ci-success`'s needs list and its separately-maintained results string are untouched. The steps ride `iced-release`, which already builds exactly the packaged configuration. The build is now one step, not two, and enforced rather than assumed: build-deb.sh's cargo invocation is byte-identical to the job's, so the two steps carry complementary conditions and exactly one runs. Previously the plan was to rely on the second being a cargo no-op against the shared target/ — true today, but an unenforced coincidence between a workflow and a script, and a silent drift would buy a cold thin-LTO relink inside a 45-minute budget. Also adds the repo's first shellcheck gate, scoped to linux/scripts/ — two new release-critical scripts had no lint anywhere. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
`ci.yml`'s rustcore path filter listed `mise.toml`, but the file is `.mise.toml` — a dotfile. The pattern could never match, so bumping the Rust or Zig toolchain triggered no Rust job at all. Pre-existing; `grep -rn "mise.toml" .github/` confirms this was the only occurrence. `build-deb.sh` cleared `dist/` but never `out/`, so repeated local builds at different versions accumulated .debs — and every consumer asserts exactly one match. This reproduced on its own during verification: the shed's `out/` still held a .deb from the previous plan, so the first build of this session produced two and resolve-one-deb.sh refused. The clear is narrow (`rm -f out/*.deb`, not `rm -rf out`) so it cannot take something a developer parked in the directory. Verified with two DISTINCT versions rather than a rerun of one: building 0.0.18-dev023a then -dev023b leaves exactly one .deb. A same-version rerun would have overwritten the same filename and passed even before the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Three adversarial-review findings on the two commits above. `ci-success` listed `changes` in its `needs` but omitted it from the `results` string it actually checks. `changes` gates every other job, so a failure there (a checkout hiccup, a paths-filter outage) skips all of them — and the allowlist accepts `skipped`, so the single required check went GREEN with nothing built. That is the same greenwash shape as the #306 incident the existing comment describes, one level further up, and it reached the release path because `release.yml`'s ci-gate trusts this check. The release smoke's `--expect-version` normalized the tag with `tr '-' '~'`, which is wrong for a tag carrying a second hyphen. nfpm's semver schema splits at the FIRST hyphen only: confirmed empirically against nfpm 2.46.3 — `1.2.3-rc-2` packages as `1.2.3~rc-2`, where `tr` produced `1.2.3~rc~2`. Since the linux job needs create-release, that mismatch would have failed a release *after* the GitHub Release was published — a failure mode this plan would have introduced. Now a first-occurrence replacement, verified for the plain, single-dash and double-dash tag shapes. The `deb` filter also gains the manifests: enabling a feature on an already-locked dependency changes what the binary dlopens without touching Cargo.lock, and the closure check is the only thing that would notice. `crates/**` stays out — source edits must not trigger a container pull. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCI and release workflows now detect Debian-related changes, build packages, resolve one architecture-specific artifact, run smoke tests, and verify dependency closure. Shared Bash helpers support these scripts. ChangesDebian packaging and release validation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CI as CI workflow
participant Build as build-deb.sh
participant Smoke as smoke-deb.sh
participant Closure as verify-deb-closure.sh
participant Release as release workflow
CI->>Build: build one Debian package
CI->>Smoke: validate packaged artifact
Smoke->>Smoke: launch Roost and poll roostctl identify
CI->>Closure: verify dependency closure
Closure->>Closure: install package in Ubuntu 24.04 container
Release->>Release: resolve one package for matrix architecture
Release->>Smoke: validate resolved package
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![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)
linux/scripts/_common.sh (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSend the
::error::line to stderr too.
dieprints the workflow command on stdout.resolve-one-deb.shis called inside a command substitution (.github/workflows/ci.yml:948,.github/workflows/release.yml:238), so on failure the::error::line is captured by the caller instead of reaching the runner's log. The annotation is then lost exactly where it is most useful. Writing both lines to stderr keeps the annotation visible and keeps stdout clean for scripts whose stdout is a value.♻️ Proposed change
die() { if [ -n "${GITHUB_ACTIONS:-}" ]; then - printf '::error::%s\n' "$*" + printf '::error::%s\n' "$*" >&2 fi printf 'error: %s\n' "$*" >&2 exit 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@linux/scripts/_common.sh` around lines 7 - 13, Update the die() function so the GitHub Actions ::error:: workflow command is written to stderr, while preserving the existing error message and exit behavior. Keep stdout free of both error lines so callers using command substitution receive only value output..github/workflows/ci.yml (1)
765-773: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winVerify the nfpm tarball checksum before installing it into
/usr/local/bin.This step downloads a release tarball and extracts it with
sudointo/usr/local/bin. The version is pinned, but a pinned tag is mutable on GitHub — a retagged or replaced asset installs a different binary with root, and that binary then builds the artifact users install.nfpmpublishes achecksums.txtalongside the release assets, so the verification is a two-line addition.This mirrors
.github/workflows/release.yml:181-194, which has the same gap. Fixing both keeps the two install paths identical, which is the stated goal of the comment on line 761.🔒 Proposed change
set -euo pipefail ver="2.46.3" url="https://github.com/goreleaser/nfpm/releases/download/v${ver}/nfpm_${ver}_Linux_x86_64.tar.gz" curl -fsSL "${url}" -o /tmp/nfpm.tgz + curl -fsSL "https://github.com/goreleaser/nfpm/releases/download/v${ver}/checksums.txt" \ + -o /tmp/nfpm.sums + ( cd /tmp && grep " nfpm_${ver}_Linux_x86_64.tar.gz\$" nfpm.sums \ + | sed 's| .*| /tmp/nfpm.tgz|' | sha256sum -c - ) sudo tar -C /usr/local/bin -xzf /tmp/nfpm.tgz nfpm nfpm --version🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 765 - 773, Update the “Install nfpm” step and the corresponding nfpm installation flow in the release workflow to download the version-matched checksums.txt, verify the downloaded nfpm tarball with the published checksum before extraction, and only then install it into /usr/local/bin. Keep both installation paths identical and preserve the existing pinned version and nfpm --version validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@linux/scripts/resolve-one-deb.sh`:
- Around line 31-35: Update the --arch handling in the argument parser to reject
both missing and empty values before assigning arch, so an empty or unset ARCH
cannot select the unfiltered package-matching path. Preserve the existing usage
error behavior and valid architecture handling.
In `@linux/scripts/smoke-deb.sh`:
- Around line 83-99: Bound the cleanup wait around APP so the EXIT trap cannot
hang indefinitely when the process group ignores SIGTERM or a child remains
alive. Follow the timeout-and-escalation pattern used by verify-deb-closure.sh:
wait only up to a fixed deadline, then run the existing kill -KILL process-group
sweep and reap the job without blocking. Preserve the normal graceful shutdown
path when APP exits within the deadline.
In `@linux/scripts/verify-deb-closure.sh`:
- Around line 104-112: Distinguish the inner UI-launch timeout from the outer
Docker timeout in the verification flow. Update the inner timeout invocation to
use a dedicated non-124 exit status, then handle that status in the case
statement with a message naming LAUNCH_TIMEOUT and the UI launch; reserve the
existing 124 branch for the outer DOCKER_TIMEOUT timeout.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 765-773: Update the “Install nfpm” step and the corresponding nfpm
installation flow in the release workflow to download the version-matched
checksums.txt, verify the downloaded nfpm tarball with the published checksum
before extraction, and only then install it into /usr/local/bin. Keep both
installation paths identical and preserve the existing pinned version and nfpm
--version validation.
In `@linux/scripts/_common.sh`:
- Around line 7-13: Update the die() function so the GitHub Actions ::error::
workflow command is written to stderr, while preserving the existing error
message and exit behavior. Keep stdout free of both error lines so callers using
command substitution receive only value output.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a65b439e-1c68-480a-bc1b-c2698ff5541a
📒 Files selected for processing (7)
.github/workflows/ci.yml.github/workflows/release.ymllinux/scripts/_common.shlinux/scripts/build-deb.shlinux/scripts/resolve-one-deb.shlinux/scripts/smoke-deb.shlinux/scripts/verify-deb-closure.sh
`resolve-one-deb.sh` accepted an empty `--arch`, which fell through to the
unfiltered branch and matched any .deb — silently disabling the very guard
the flag exists for. release.yml passes `"${ARCH}"` from the matrix, so an
unset ARCH could have handed an arm64 package to the job that labels it
Intel/AMD.
`smoke-deb.sh`'s cleanup did an unbounded `wait` before its KILL sweep, so
the sweep was unreachable in exactly the case it existed for: anything in
the process group ignoring SIGTERM would block the EXIT trap until the job
timeout. Same hang class this plan already fixed in the closure script, on
the same release-gating path. Now a bounded poll, then KILL, then reap.
The closure script's exit-code mapping conflated its two timeouts. Both
`timeout` calls exit 124 and the inner one propagates out as the container
status, so a launch timeout reported itself as the 900s docker budget
being exceeded — the same misdiagnosis this script was rewritten to stop
making, one layer down. The inner timeout is now remapped to 125 with its
own message.
All three verified in the shed: empty `--arch` rejected; smoke and closure
still pass on a good .deb; and the new 125 path exercised end-to-end
against the stripped-Depends .deb with a short launch budget, which
correctly reports "did not answer roostctl identify within 5s ... the
package DID install" instead of blaming the docker budget.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
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.
Plan 023, workstream C (+ two of the three W-D bugs). Part 2 of 3; PR #316
(badge) is merged, docs PR to follow.
The problem
release.yml's two newest steps — the packaged-artifact smoke and theclean-container dependency-closure check — only fire on a
v*tag, in a jobthat
needs: create-release. So they first execute after the GitHub Releaseis already published, and they had never run in GitHub Actions at all. If
either was broken, the first person to find out would be Charlie, mid-release.
What changed
Extracted, not copied.
linux/scripts/{resolve-one-deb,smoke-deb,verify-deb-closure}.sh(all
100755).release.ymlandci.ymlnow call the same scripts, so whatCI proves is what the release runs.
resolve-one-deb.shexists because a<deb-path>contract can't host the"exactly one deb" assertion, and leaving that in the callers would mean the
release steps still contained logic. All three call sites share it.
Behaviour preserved, with four deliberate widenings: the payload check now
covers all seven
packaging/nfpm.yamldestinations rather than the twobinaries;
dpkg-deb -fasserts Package/Architecture/Version, which makesnfpm's
-→~normalization checked instead of assumed;ROOST_BUNDLE_PROFILEis explicitly unset (merely never setting it isn't the same thing when the
script is run by hand); and each script owns its XDG sandbox — load-bearing,
not hygiene, because in CI these run after steps that already launched a UI, so
without it the smoke could get a successful
identifyfrom an earlier instanceand pass while proving nothing.
CI runs them on a narrow new
debfilter. The obvious choice — the existinglinuxfilter — folds in the*rustcoreanchor and is therefore true onessentially every Rust PR, which would have meant a Docker Hub pull per PR. The
new filter is only the paths that can change what the
.debcontains. It alsoincludes
release.yml, which appeared in no filter before, so a PR editingonly the release workflow triggered nothing capable of validating it.
One build, not two — enforced.
build-deb.sh's cargo invocation isbyte-identical to
iced-release's, so the two build steps now carrycomplementary conditions and exactly one runs. The alternative was relying on
the second being a cargo no-op against the shared
target/: true today, but anunenforced coincidence between a workflow and a script, and a silent drift buys
a cold thin-LTO relink inside a 45-minute budget.
Plus the repo's first
shellcheckgate (scoped tolinux/scripts/), and twoW-D bugs:
ci.yml's rustcore filter matchedmise.tomlwhen the file is.mise.toml, so a toolchain bump triggered no Rust job; andbuild-deb.shcleared
dist/but neverout/.What running it actually found
This is the part that justifies the workstream.
1. The closure check hung for 31 minutes and had to be killed by hand — the
logic exactly as it exists on
maintoday. Its only backstop is the job's90-minute timeout, burned after the Release is public. Now bounded with
timeouton both thedocker runand the in-container launch, plus an explicitreap of the backgrounded UI.
timeoutalso puts xvfb-run in its own processgroup, which is what stops the background UI holding the container's streams
open. 31 minutes → 8.6 seconds.
2. Its failure message asserted the wrong cause. It reported
"its Depends: list is incomplete"for what was actually a hang — theinstrumented rerun proved the closure was fine. That message would have sent
someone hunting a dependency bug that did not exist. It now distinguishes a
timeout (harness/environment) from a non-zero exit (likely, but not certainly,
the closure).
3. A release-failing version bug I was about to introduce. The smoke's
--expect-versionnormalized the tag withtr '-' '~'. nfpm's semver schemasplits at the first hyphen only — confirmed empirically against nfpm 2.46.3:
1.2.3-rc-2packages as1.2.3~rc-2, wheretrgives1.2.3~rc~2. Aprerelease tag with two hyphens would have failed the release after
create-releasepublished. Fixed and verified for plain, single-dash anddouble-dash tags.
4. A live greenwash of the required check (pre-existing).
ci-successlisted
changesinneedsbut omitted it from theresultsstring itactually checks.
changesgates every other job, so a failure there skips allof them — and the allowlist accepts
skipped, so the single required checkgoes green with nothing built. Same shape as the #306 incident its own
comment describes, one level further up, and it reached the release path
because
release.yml'sci-gatetrusts this check. One-line fix; called outbecause it is the kind of thing that only shows up on a bad day.
5. The
out/bug reproduced on its own — the shed'sout/still held a.debfrom plan 022, so the first build produced two andresolve-one-deb.shrefused. Found before the fix for it was written.
Verification
Shed (Ubuntu 24.04 arm64), against a real
.deb. Full transcript in theplan's artifact folder.
Positive: smoke passes (all 7 destinations, production
roostnamespace,version assertion); closure passes in 8.6s;
build-deb.shat two distinctversions leaves exactly one
.deb.Negative controls, so a green run means something:
linux-package, repackage, smoke…/roost-iced/roost.sockDepends:, run closurelibXcursor.so.1: cannot open shared object file— proves the list is load-bearingusr/share/doc/roost/README.md missing inside ….debs inout/--expect-version 9.9.9The D4.3 double-build check uses two distinct versions on purpose: a
same-version rerun overwrites the same filename and would have passed even
before the fix.
Known limits (recorded, not hidden)
.debwas built or installed locally;this PR's CI run is the first amd64 exercise, and the release job the first
amd64 install. arm64 remains CI-unvalidated — filed as a follow-up issue.
release.yml's tag trigger, jobgraph, or matrix — those are still first exercised by a real tag.
debfilter excludescrates/**by design (source edits must nottrigger a container pull); manifests are included, since a feature flip can
change the dlopen set without touching
Cargo.lock..debis built. This PRlowers the chance of a failed artifact build but cannot prevent a published
release with missing assets; making
create-releaseproduce a draft is arelease-workflow change beyond this plan's scope.
Impact
No new dependency. No new CI job, so
ci-success'sneedslist is unchanged(its
resultsstring gainschanges— that's the greenwash fix). No privacyor secret impact.
Plan 023 §D3 — the pinned design (excerpt)
D3.1 — three scripts. Strict argument parsing (a bare positional second
argument would let a glob that expanded to two files be read as
<deb> <work-dir>); each script owns its XDG sandbox; cleanup terminates theactual UI via its process group and removes only directories it created, never
a caller-supplied
--work-dir;die()emits::error::only underGITHUB_ACTIONSso shed output stays readable.D3.2 — a narrow
debfilter; the first draft's gating claim was false. Thefirst draft gated on
linux || ciand justified the cost by saying ordinaryRust PRs would not pay it.
linuxincludes therustcoreanchor, so it is trueon every
crates/**PR — including this plan's own PR 1. The Docker Hub pullwould have moved from "on tags" to "on every Rust PR".
D3.3 — one build invocation, chosen by the same filter. Removed the drift
hazard rather than guarding it: the two builds are mutually exclusive.
D3.4 — scope claim corrected. This exercises the release logic, not the
YAML wiring around it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SgxiEBQTqgNjPswKqcs12d
Summary by CodeRabbit
New Features
.debrelease validation, including architecture checks, clean-environment installation testing, and dependency verification.Bug Fixes