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[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@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.
sheensantoscapadngan
requested review from
bernie-g
and removed request for
sheensantoscapadngan
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 33062794 | Triggered | Generic CLI Secret | 997dd5f | packages/cmd/login_status_test.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
1 participant
Description 📣
Add CLI smoke tests across multiple distros
Type ✨
Tests 🛠️
# Here's some code block to paste some code snippets