Conversation
WalkthroughAdds/updates CI workflows and release tooling, introduces a Makefile and golangci config, refactors CLI command wiring into constructor functions, enhances CA certificate timeout/metadata handling, tightens I/O/error handling, and restructures message/webhook command validation and rendering. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/commands/ca/common/utils.go (1)
80-86:⚠️ Potential issue | 🟡 MinorZero or negative timeout causes immediate failure.
If
c.Duration("timeout")returns0(e.g., user passes--timeout 0),time.After(0)fires immediately, and the loop will exit with "Timeout waiting for certificate" before ever polling. Consider adding a guard or documenting the minimum acceptable value.
🤖 Fix all issues with AI agents
In @.github/workflows/pr.yml:
- Around line 115-125: The Docker link in the PR comment row that currently uses
the string "https://${{ env.DOCKER_REGISTRY }}/${{ env.PROJECT_NAME }}:pr-${{
github.event.pull_request.number }}" produces an invalid web URL because it
includes a Docker tag; update the 🐳 Docker table cell in
.github/workflows/pr.yml to either (A) link to the package page without the tag
by removing the ":pr-..." suffix (e.g. use "https://${{ env.DOCKER_REGISTRY
}}/${{ env.PROJECT_NAME }}" or simply "https://${{ env.DOCKER_REGISTRY }}"
depending on how DOCKER_REGISTRY/PROJECT_NAME are set) or (B) render the pull
reference as plain text/code (e.g. `docker pull ${{ env.DOCKER_REGISTRY }}/${{
env.PROJECT_NAME }}:pr-${{ github.event.pull_request.number }}`) instead of a
hyperlink so the tag remains usable for consumers.
In @.golangci.yml:
- Around line 21-37: The golangci configuration currently enables the "swaggo"
formatter in the formatters.enable list; remove "swaggo" from the enable block
so the formatter list only contains active formatters (e.g., "goimports" and
"golines"). Update the formatters.enable array to drop the "swaggo" entry to
eliminate the unused formatter from the config.
In @.goreleaser.yaml:
- Around line 50-52: The images template uses {{ .Env.DOCKER_REGISTRY }} which
will fail if DOCKER_REGISTRY is unset; update the images entry to use a safe
fallback or validate the env var before release: replace or change the "{{
.Env.DOCKER_REGISTRY }}/{{ .ProjectName }}" template to include a default
registry (e.g., use a fallback like "ghcr.io/android-sms-gateway/{{ .ProjectName
}}" or implement a validation step in your release workflow that ensures
DOCKER_REGISTRY is set) and ensure the change touches the images list in
.goreleaser.yaml referencing {{ .Env.DOCKER_REGISTRY }} and {{ .ProjectName }}.
In `@internal/commands/ca/common/utils.go`:
- Line 11: The import line currently has a TODO referencing the wrong project
("mariadb-backup-s3") next to the "log" import; update that TODO to reference
the correct logger replacement plan for this CLI (or remove it if not needed) so
it no longer points to an unrelated project—locate the import statement
containing "log" and the trailing comment "// TODO: replace with logger from
mariadb-backup-s3" and change the comment to the appropriate project/logger name
or a neutral TODO like "// TODO: replace with shared CLI logger" for clarity.
In `@internal/commands/messages/send.go`:
- Around line 107-117: sendBefore currently only checks TTL for mutual
exclusivity but allows negative durations, which later cause uint64 underflow in
sendAction when converting ttl via uint64(c.Duration("ttl").Seconds()); add a
non-negative check in sendBefore to return a ParamsError if ttl < 0, and
additionally add a defensive guard in sendAction before converting TTL (check
c.Duration("ttl") >= 0) to prevent converting a negative duration to uint64;
reference sendBefore and sendAction and the use of
c.Duration("ttl")/uint64(...).
In `@internal/commands/webhooks/delete.go`:
- Around line 26-29: metadata.GetClient and metadata.GetRenderer can return nil
and the code in DeleteWebhook (client := metadata.GetClient(c.App.Metadata);
renderer := metadata.GetRenderer(c.App.Metadata); err :=
client.DeleteWebhook(c.Context, id)) uses them without checks; add nil guards
after retrieving client and renderer in DeleteWebhook (and mirror the same
checks in list.go, register.go, etc.): if client or renderer is nil, return or
exit with a clear error/panic message via the command's error handling (e.g.,
return fmt.Errorf(...) or renderer.Error/cli context error) instead of calling
methods on a nil pointer, ensuring the DeleteWebhook call is only invoked when
client is non-nil.
In `@Makefile`:
- Around line 48-51: The Makefile's build target is invoking go build from the
repo root where no main package exists; update the build target (target name:
build) to explicitly compile the two main entrypoints by running go build on
./cmd/smsgate/smsgate.go and ./cmd/smsgate-ca/smsgate-ca.go (producing
bin/smsgate and bin/smsgate-ca), keeping the mkdir -p bin step and appropriate
-o outputs so both binaries are built into the bin directory.
🧹 Nitpick comments (12)
internal/commands/webhooks/delete.go (1)
34-38: Empty output line printed for JSON format on success.
JSONOutput.Success()returns("", nil).fmt.Fprintln(os.Stdout, "")will write a blank line to stdout, which may be undesirable for JSON consumers piping output.Consider skipping the print when
bis empty:Proposed fix
b, err := renderer.Success() if err != nil { return cli.Exit(err.Error(), codes.OutputError) } - fmt.Fprintln(os.Stdout, b) + if b != "" { + fmt.Fprintln(os.Stdout, b) + } return nilinternal/commands/webhooks/register.go (1)
64-67: Nit:c.String("device-id")is called twice.Minor redundancy — the flag value is fetched twice. Could use a local variable.
Suggested tweak
- var deviceID *string - if c.String("device-id") != "" { - deviceID = lo.ToPtr(c.String("device-id")) - } + var deviceID *string + if did := c.String("device-id"); did != "" { + deviceID = lo.ToPtr(did) + }cmd/smsgate-ca/smsgate-ca.go (1)
32-32:ca.Commands()is called twice — commands are constructed and discarded for the capacity hint.
ca.Commands()on line 32 builds the full command slice just to measure its length, then discards it. Line 57 calls it again to get the actual commands. Store the result once:♻️ Proposed fix
+ cmds := ca.Commands() + app := &cli.App{ Name: "smsgate-ca", Usage: "CLI interface for interacting with Certificate Authority of SMSGate", Version: version, - Commands: make([]*cli.Command, 0, len(ca.Commands())), + Commands: make([]*cli.Command, 0, len(cmds)), Flags: []cli.Flag{- app.Commands = append(app.Commands, ca.Commands()...) + app.Commands = append(app.Commands, cmds...)Also applies to: 57-57
cmd/smsgate/smsgate.go (1)
32-32: Same double-invocation issue:Commands()called twice for each module.Same pattern as
smsgate-ca.go—messages.Commands()andwebhooks.Commands()are each called on line 32 for the capacity hint, then again on lines 97–98 for the actual append. Store them once:♻️ Proposed fix
+ msgCmds := messages.Commands() + whCmds := webhooks.Commands() + app := &cli.App{ Name: "smsgate", Usage: "CLI interface for working with SMS Gateway for Android™", Version: version, - Commands: make([]*cli.Command, 0, len(messages.Commands())+len(webhooks.Commands())), + Commands: make([]*cli.Command, 0, len(msgCmds)+len(whCmds)),- app.Commands = append(app.Commands, messages.Commands()...) - app.Commands = append(app.Commands, webhooks.Commands()...) + app.Commands = append(app.Commands, msgCmds...) + app.Commands = append(app.Commands, whCmds...)Also applies to: 97-98
.github/workflows/close-issues-prs.yml (1)
1-25: Consider adding exempt labels and extending the stale window.A 7-day stale + 7-day close window (14 days total) is quite aggressive — legitimate issues/PRs waiting on maintainer feedback or contributor availability may be auto-closed. Also, there are no
exempt-issue-labelsorexempt-pr-labelsconfigured, so even confirmed bugs or pinned items will be marked stale.Consider:
- Extending
days-before-staleto at least 30 days.- Adding
exempt-issue-labels/exempt-pr-labelsfor labels like"bug","pinned", or"in-progress"..github/workflows/pr-labels.yml (1)
13-15: The checkout step is unnecessary here.The workflow only runs
gh pr editcommands, which don't require the repository to be checked out —ghonly needsGH_TOKENand the repo context (already available viagithubcontext). Removing the checkout step would shave ~5-10 seconds off each run.Proposed fix
steps: - # step 1: checkout repository code - - name: Checkout code into workspace directory - uses: actions/checkout@v4 - # step 2: remove specific labels - name: Remove specific labels.github/workflows/release.yml (1)
40-42: Remove commented-out code.The commented-out
RELEASE_IDblock adds noise. If it's not needed, remove it; if it's planned for future use, track it in an issue instead.Proposed cleanup
- # RELEASE_ID: Days since project inception (2026-01-01) - # - name: Set RELEASE_ID env - # run: echo RELEASE_ID=$(( ($(date +%s) - $(date -d "2026-01-01" +%s)) / 86400 )) >> ${GITHUB_ENV} - - name: Run GoReleaser.github/workflows/go.yml (2)
44-44:golangci-lint-action@v8works butv9is already available.v8.0.0 requires golangci-lint version >= v2.1.0, which aligns with the v2 config in
.golangci.yml. However, v9.0.0 changes the Nodejs runtime from node20 to node24 (released November 2025). GitHub is deprecating node20 runners, so you may want to bump tov9proactively to avoid future deprecation warnings.
77-131: Benchmark job is well-structured. Consider enablingcomment-on-alertfor PR visibility.The benchmark job grants
pull-requests: write(Line 82) but thebenchmark-actionstep (Lines 113–123) does not setcomment-on-alert: true. If the intent behind requestingpull-requests: writeis to allow the action to post regression comments on PRs, you'll want to enable it explicitly since the default isfalse.Also, the
if: always()on cache save (Line 127) correctly persists the benchmark history even whenfail-on-alert: truetriggers a failure — this preserves the historical data for subsequent runs.Proposed diff to enable PR comments
- name: Upload benchmark results uses: benchmark-action/github-action-benchmark@v1 with: # What benchmark tool the benchmark.txt came from tool: "go" # Where the output from the benchmark tool is stored output-file-path: benchmark.txt # Where the previous data file is stored external-data-json-path: ./cache/benchmark-data.json # Workflow will fail when an alert happens fail-on-alert: true + # Post a comment on the PR when an alert happens + comment-on-alert: true + github-token: ${{ secrets.GITHUB_TOKEN }}.golangci.yml (2)
1-13: Config header references v2.5.0, but latest golangci-lint is v2.9.0.Line 4 states "Golden config for golangci-lint v2.5.0" while golangci-lint v2.9.0 was released on 2026-02-10. The config is forward-compatible, but the comment may mislead future maintainers into thinking the config is pinned or tested against that specific version. Consider updating the comment or removing the version reference.
234-292: Theexhaustructexclusion list contains many libraries likely not used by this project.Exclusions for Firebase, Telegram bots, Sarama, AWS SDK, Prometheus, etc. are no-ops if those packages aren't imported, but they add significant noise. Since this config originates from a community template, consider trimming the exclusion list to only the dependencies actually used (e.g.,
cobra,testify,urfave/cli)..github/workflows/pr.yml (1)
50-52: Commented-outRELEASE_IDblock.If this is no longer needed, consider removing it entirely rather than leaving it commented out. If it's planned for future use, a TODO comment explaining the intent would be helpful.
🤖 Pull request artifacts
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/pr.yml:
- Around line 116-123: Replace the use of the secret AWS_BUCKET in the PR
comment body (currently referenced as ${{ secrets.AWS_BUCKET }}) with the
repository variable reference ${{ vars.AWS_BUCKET }} so the bucket hostname is
stored as a non-secret repo variable; create/update the repository variable
named AWS_BUCKET accordingly and update all URL templates that use ${{
secrets.AWS_BUCKET }} to ${{ vars.AWS_BUCKET }} (pattern similar to existing
DOCKER_REGISTRY usage) to avoid exposing secret values in rendered PR comments.
In `@internal/commands/messages/send.go`:
- Around line 107-111: The validation in sendBefore currently checks sim via sim
< 0 || sim > 255 but the error text "SIM card index must be between 1 and 255"
is misleading because 0 is allowed/used as default; update the error message in
sendBefore to reflect that 0 is valid (e.g., "SIM card index must be between 0
and 255 (0 for default)") so callers understand 0 means unset/default while
keeping the same numeric bounds check.
- Around line 168-169: Add nil guards after obtaining client :=
metadata.GetClient(c.App.Metadata) and renderer :=
metadata.GetRenderer(c.App.Metadata): check if client == nil or renderer == nil
and return a helpful error (or call c.Errf/exit) before any use. Specifically,
guard before calling client.Send(...) and renderer.MessageState(...), and
include contextual messages referencing the missing metadata key so callers know
which dependency is misconfigured; apply the same pattern to the client and
renderer variables in SendCommand (send.go) to avoid nil-pointer panics.
🧹 Nitpick comments (4)
cmd/smsgate-ca/smsgate-ca.go (1)
35-46:Required: falseis the default for urfave/cli flags — can be omitted.Diff
&cli.DurationFlag{ Name: "timeout", Aliases: []string{"t"}, Usage: "Request timeout", - Required: false, Value: defaultTimeout, EnvVars: []string{ "ASG_CA_TIMEOUT", }, },.github/workflows/pr.yml (2)
19-22: Unusedrepositoryvariable in Prepare step.Line 21 assigns
repository=${{ github.repository }}but it's never referenced. This looks like leftover code from an intent to dynamically derivePROJECT_NAMEfrom the repository name, but the value was hardcoded tosmsgateinstead.If hardcoding is intentional, remove the unused variable. If dynamic derivation was intended:
♻️ Suggested fix
- name: Prepare run: | - repository=${{ github.repository }} - echo "PROJECT_NAME=smsgate" >> $GITHUB_ENV + echo "PROJECT_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV
50-52: Remove commented-out code.The commented-out RELEASE_ID block is dead code. If it's not needed, remove it to keep the workflow clean. If it's planned for future use, track it in an issue instead.
internal/commands/messages/send.go (1)
216-216: Prefervardeclaration for initially-empty slice.
[]smsgateway.SendOption{}allocates backing memory immediately. Avar options []smsgateway.SendOption(nil slice) is idiomatic Go for slices that may remain empty, andappendworks fine on nil slices.Suggested change
- options := []smsgateway.SendOption{} + var options []smsgateway.SendOption
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/go.yml:
- Around line 77-80: The benchmark job definition named "benchmark" currently
uses the constant condition if: false which triggers actionlint; either remove
the entire "benchmark" job block or replace the conditional by commenting the
job out and/or add a clear comment above the job explaining why it's disabled;
specifically update the "benchmark" job entry (job name "benchmark", runs-on and
if keys) to eliminate the constant if: false so the linter no longer flags it.
capcom6
force-pushed
the
dev/update-tooling
branch
from
abf0d63 to
d6daffb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/pr.yml:
- Around line 79-91: The workflow's "Push images" step is failing because
GoReleaser's multi-platform build does not produce per-arch tags like
latest-arm64/latest-amd64, so the docker tag commands referencing
latest-arm64/latest-amd64 and the subsequent docker manifest creation will
error; fix by either (A) removing the re-tagging and manifest assembly in the
"Push images" step and push/use the manifest that GoReleaser already produces
(use the tag names GoReleaser emits such as {{ .Tag }}, v{{ .Major }}, v{{
.Major }}.{{ .Minor }}, latest), or (B) modify .goreleaser.yaml to emit
architecture-specific tags (e.g., append arch to the image tags so
latest-amd64/latest-arm64 are created) and then keep the existing docker
tag/manifest logic; update the "Push images" step accordingly to match whichever
approach you choose.
🧹 Nitpick comments (4)
cmd/smsgate/smsgate.go (1)
28-34: Command assembly is correct but relies on a subtle append behavior.
append(messages.Commands(), webhooks.Commands()...)works correctly here becausemessages.Commands()returns a slice literal (len == cap), so append always allocates. However, ifCommands()ever returns a slice with spare capacity, this would silently mutate the original. A more explicit construction avoids this coupling.Optional: explicit slice construction
- cmds := append(messages.Commands(), webhooks.Commands()...) + cmds := make([]*cli.Command, 0, len(messages.Commands())+len(webhooks.Commands())) + cmds = append(cmds, messages.Commands()...) + cmds = append(cmds, webhooks.Commands()...).github/workflows/pr-labels.yml (1)
14-15: Checkout step is unnecessary here.The
gh pr editcommand only needsGH_TOKENfor authentication; it doesn't require repository code to be checked out. TheghCLI is pre-installed on GitHub-hosted runners. Removing this step would speed up the workflow.♻️ Suggested diff
steps: - # step 1: checkout repository code - - name: Checkout code into workspace directory - uses: actions/checkout@v4 - # step 2: remove specific labels - name: Remove specific labels.github/workflows/close-issues-prs.yml (1)
15-16: 7-day stale + 7-day close window is aggressive.Issues and PRs will be auto-closed after just 14 days of inactivity. For a CLI tool where contributors may work intermittently, this could prematurely close valid issues/PRs. Consider extending to at least 30+14 or 60+14 days, which is more common for open-source projects.
.github/workflows/pr.yml (1)
19-22: Dead code:repositoryvariable is set but never used.Line 21 captures
github.repositoryinto a shell variablerepository, but it's never referenced —PROJECT_NAMEis hardcoded to"smsgate"on line 22. Either remove the unused variable or use it to derivePROJECT_NAMEdynamically.♻️ Option A: Remove unused variable
- name: Prepare run: | - repository=${{ github.repository }} echo "PROJECT_NAME=smsgate" >> $GITHUB_ENV♻️ Option B: Derive PROJECT_NAME from repository
- name: Prepare run: | - repository=${{ github.repository }} - echo "PROJECT_NAME=smsgate" >> $GITHUB_ENV + echo "PROJECT_NAME=${GITHUB_REPOSITORY##*/}" >> $GITHUB_ENV
capcom6
force-pushed
the
dev/update-tooling
branch
from
d173076 to
db1f299
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/smsgate/smsgate.go (1)
28-30:Commands()called twice — commands constructed and discarded for the capacity hint.
messages.Commands()andwebhooks.Commands()are each called twice: once forlen()(Line 28) and once forappend(Lines 29-30). Since these are constructor functions, the first call allocates command objects that are immediately discarded. Consider storing the results first:♻️ Suggested fix
- cmds := make([]*cli.Command, 0, len(messages.Commands())+len(webhooks.Commands())) - cmds = append(cmds, messages.Commands()...) - cmds = append(cmds, webhooks.Commands()...) + msgCmds := messages.Commands() + whCmds := webhooks.Commands() + cmds := make([]*cli.Command, 0, len(msgCmds)+len(whCmds)) + cmds = append(cmds, msgCmds...) + cmds = append(cmds, whCmds...)internal/commands/ca/common/utils.go (1)
41-41: ExplicitHeaders: nilis redundant but harmless.Both
pem.Blockliterals now includeHeaders: nil, which is the zero value. This is fine for clarity/consistency — just noting it's not functionally necessary.Also applies to: 56-59
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores