fix(slack-bot): harden release image - #1182
Conversation
6ddf748 to
b607cab
Compare
jhrozek
left a comment
There was a problem hiding this comment.
Panel review (Spec / Standards / Test-adequacy / Domain specialists) on PR #1182. Solid hardening work overall (multi-stage build, digest-pinned Chainguard base matching .ko.yaml convention, non-root, shell removed, no baked-in secrets) — one blocker on unmet issue scope, plus a few important/advisory findings below.
Co-Authored-By: Mecatl <mecatl@stacklok.com>
b607cab to
4c9ffc5
Compare
Co-Authored-By: Mecatl <mecatl@stacklok.com>
jhrozek
left a comment
There was a problem hiding this comment.
Follow-up panel review after the latest fix commit (919d3117e).
The blocker from the previous review is fixed correctly: the new CI step spawns a real offline mecated --mock daemon and runs the actual built/hardened image's MecatlBridge against it over gRPC, asserting a genuine response — closing the "never tested against the built image" gap, with an honest comment scoping out Socket Mode itself (which needs live Slack credentials, out of reach for CI). Verified no new issues from this addition:
- The step lives in the
slack-bot-examplejob onubuntu-24.04— a real Linux runner, sodocker run --network hostbehaves as intended, not Docker Desktop's VM-isolated networking. - The
trap ... EXITis registered immediately after backgrounding the daemon, minimizing any leak window; the readiness poll loop is correctly bounded (10s) and fails fast viakill -0if the daemon dies early. - The hardcoded mock-provider response string is a pre-existing coupling already shared by two other test files, not a new fragility introduced here.
- No new security surface —
--network hostonly affects this ephemeral CI job, and the mock daemon needs no credentials.
All three remaining advisories are also fixed correctly: Dependabot gained a docker ecosystem entry scoped to the right directory, .dockerignore now excludes **/.npmrc, and the builder FROM line dropped the moving latest-dev tag, keeping only the digest (matching .ko.yaml's convention).
Approving.
Co-Authored-By: mecatl <noreply@stacklok.com>
jhrozek
left a comment
There was a problem hiding this comment.
Re-reviewed the changes since my last approval — the streaming/cancel/stuck-auth-eviction work addressing the other review round looks correct and is well covered by new tests. The one failing check (Test (race root-a), internal/app) is a pre-existing flake on main unrelated to this change, not something introduced here. LGTM.
Summary
Verification
task linttask testtask docstask site:buildtask lint:actionsFixes #1085