Conversation
|
💬 Discussion in Slack: #pr-review-cli-239-feat-cli-add-smoke-tests-across-multiple-distros Posted by Review Police — reviews, comments, new commits, and CI failures will stream into this channel. |
|
@claude review this PR once |
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, comment @claude review on this pull request to trigger a review.
|
@claude review this once |
There was a problem hiding this comment.
LGTM — all prior comments are addressed (paths corrected, nfpm pinned to v2.46.3, ldd skip message clarified).
Extended reasoning...
Overview
This PR adds a CI workflow (run-cli-smoke-tests.yml) plus supporting files (smoke-tests/nfpm.yaml and smoke-tests/smoke.sh) that build the CLI as a static binary, package it via nfpm into deb/rpm/apk/archlinux formats, and run a shell-based smoke test (binary presence, --version, --help, core subcommand --help, static-binary check) across an 8-distro Docker matrix. No production code is touched.
Security risks
Minimal. The workflow triggers on pull_request, which means it doesn't get repo secrets, and it runs Docker on the GitHub-hosted ubuntu runner against pinned-version actions. The third-party tooling that gets fetched at run-time (nfpm) is now pinned to a specific tag (v2.46.3), eliminating the supply-chain concern from the prior revision. Container images in the matrix (e.g. alpine:3.20, fedora:40) are unpinned by tag, which is a minor reproducibility consideration but standard practice for smoke tests of this kind.
Level of scrutiny
Low. This is test-infrastructure code that does not ship to users, has no auth/crypto/permissions implications, and runs in an isolated CI context. The changes are mechanical and self-contained — well within the bar for shadow approval.
Other factors
The author explicitly requested a re-review after addressing my prior comments. Cross-checking the diff against those comments: the critical path mismatch (artifacts/smoke-tests/... → artifacts/...) is fixed, nfpm@latest is now nfpm@v2.46.3, and the misleading PASS: no missing shared libraries line is replaced with an explicit static binary (no dynamic dependencies) branch. The bug hunting system found no new issues on this revision.
Description 📣
Add CLI smoke tests across multiple distros
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets