V0.0.13#55
Conversation
Updated Go version to 1.23.4 across workflows, Dockerfile, and `go.mod`. Enhanced error handling in consumers by switching to `RunE` and checking for context cancellation. Adjusted NATS queue defaults and removed unused configuration keys for cleaner setup.
Refactored consumer processes to improve error handling by wrapping deferred unsubscribes and separating logic into smaller methods. Updated workflows, Makefile, and Docker-related tags to version 0.0.12. Documentation and error logging enhancements were also added for better clarity and maintainability.
|
Caution Review failedThe pull request is closed. WalkthroughBumps Go/tooling versions and Docker images; updates GolangCI configuration to v2 format; refactors generator and validator services to use value receivers, per-message run helpers, and context-aware loops; changes a Cobra command to RunE with error propagation; adds NATS reply stubs and unit tests for generator/validator flows. Changes
Sequence Diagram(s)sequenceDiagram
participant NATS as NATS
participant Gen as Generator
participant Pwd as PasswordSvc
Note over Gen: Process(ctx, delivery)
NATS-->>Gen: *nats.Msg (Generate)
Gen->>Gen: run(ctx, msg)
Gen->>Gen: Unmarshal Generate
Gen->>Pwd: HashPassword(password)
Pwd-->>Gen: hash, salt or error
alt success
Gen-->>NATS: Respond(TypeGenerated, Generated{hash,salt})
else error
Gen-->>NATS: Respond(TypeGeneratedError)
end
Note over Gen: exits on ctx.Done() or error
sequenceDiagram
participant NATS as NATS
participant Val as Validator
participant Pwd as PasswordSvc
Note over Val: Process(ctx, delivery)
NATS-->>Val: *nats.Msg (Validate)
Val->>Val: run(ctx, msg)
Val->>Val: Unmarshal Validate
Val->>Pwd: IsValid(password, hash, salt)
Pwd-->>Val: true/false or error
alt valid
Val-->>NATS: Respond(TypeValid)
else invalid
Val-->>NATS: Respond(TypeInvalid)
else error/unmarshal
Val-->>NATS: Respond(TypeValidatedError)
end
Note over Val: exits on ctx.Done() or error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (13)
.github/workflows/main.yml (1)
88-99: De-duplicate Docker tag via a single env variable; reduce drift across steps.Hardcoding the tag in two places is easy to miss in future bumps. Recommend an env-scoped IMAGE_TAG and reuse it.
Apply this diff within the changed section:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.12 + tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ env.IMAGE_TAG }}And in the “Build and push” step:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.12 + tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ env.IMAGE_TAG }}Add this (outside the changed section) to the top-level env block:
env: DO_PUSH: ${{ github.ref == 'refs/heads/main' }} IMAGE_TAG: pass-0.0.12Dockerfile (1)
14-24: Add CA certificates and a basic HEALTHCHECK to improve runtime resilience and ops visibility.
- Alpine images don’t include CA certs by default; outbound HTTPS will fail without them.
- Add a minimal HEALTHCHECK if the service exposes a status endpoint.
Apply this diff within the changed block:
FROM alpine:3.21 RUN adduser -S www-data -G www-data + +# TLS trust store for outbound HTTPS +RUN apk add --no-cache ca-certificates @@ USER www-data - -CMD ["/bin/stockwayup"] +# +# Optional: if your service provides a status/health endpoint, uncomment and adjust: +# HEALTHCHECK --interval=30s --timeout=3s --retries=3 CMD wget -q -O- http://127.0.0.1:8080/health || exit 1 +CMD ["/bin/stockwayup"]If you share the service’s health/metrics port and endpoint path, I can tailor the HEALTHCHECK command.
.golangci.yml (1)
12-35: Broad global suppressions undermine “enable-all: true”; scope them to generated code onlyThe catch-all exclusion (path: .*) disables many useful linters across the entire repo (gofumpt, gci, wrapcheck, exhaustruct, varnamelen, funlen, etc.). This neutralizes enable-all and risks missing real issues.
Recommend narrowing exclusions to generated code paths (e.g., transport/event/*_gen.go) and specific noise sources.
Apply a narrower rule set like:
issues: exclude-rules: - path: _test\.go linters: - funlen - dupl - - path: .* - linters: - - gci - - gofumpt - - exhaustivestruct - - exhaustruct - - wrapcheck - - tagliatelle - - dupl - - varnamelen - - nonamedreturns - - funlen - - depguard + - path: ^transport/event/.*_gen(_test)?\.go$ + linters: + - exhaustivestruct + - exhaustruct + - wrapcheck + - tagliatelle + - varnamelen + - nonamedreturns + - funlen + - dupl + - depguardIf there are other generated folders, we can add them explicitly rather than disabling linters repo-wide.
cmd/generate_consumer.go (5)
45-49: RunE should return errors instead of exiting the process (defers won't run with os.Exit).Switch to returning the error so Cobra can propagate it and your defers (Close/Unsubscribe) still execute.
- if err != nil { - logger.Err(err).Msg("nats failed to establish connection") - os.Exit(1) - } + if err != nil { + logger.Err(err).Msg("nats failed to establish connection") + return err + }
55-59: Return subscribe errors; don’t os.Exit inside RunE.Same rationale as connection errors: preserve defers and allow upstream handling.
- if err != nil { - logger.Err(err).Msg("nats failed to subscribe") - os.Exit(1) - } + if err != nil { + logger.Err(err).Msg("nats failed to subscribe") + return err + }
50-51: Consider draining the NATS connection for graceful shutdown.Drain flushes pending publishes/subscribes before closing. Optional but safer in production.
- defer natsConn.Close() + defer func() { + _ = natsConn.Drain() + }()
63-68: Avoid logging an error entry when err is nil.This currently emits a log with a nil error on graceful exit.
- g.Go(func() error { - err := service.NewGenerator(service.NewPasswordSvc(cfg)).Process(ctx, mch) - - logger.Err(err).Msg("generator process") - return err - }) + g.Go(func() error { + err := service.NewGenerator(service.NewPasswordSvc(cfg)).Process(ctx, mch) + if err != nil { + logger.Err(err).Msg("generator process") + } + return err + })
3-16: Treat delivery channel closure as a non-fatal shutdown cause.Process returns dictionary.ErrDeliveryChannelClosed when the delivery channel closes. Handle it like context cancellation.
import ( "context" "errors" "os" "github.com/nats-io/nats.go" + "github.com/stockwayup/pass/dictionary" "github.com/stockwayup/pass/transport" "github.com/rs/zerolog" "github.com/spf13/cobra" "github.com/stockwayup/pass/conf" "github.com/stockwayup/pass/service" "golang.org/x/sync/errgroup" ) @@ - err = g.Wait() - if err == nil || errors.Is(err, context.Canceled) { + err = g.Wait() + if err == nil || + errors.Is(err, context.Canceled) || + errors.Is(err, dictionary.ErrDeliveryChannelClosed) { return nil } logger.Err(err).Msg("wait goroutines") return errAlso applies to: 70-77
cmd/validate_consumer.go (5)
47-50: RunE should return errors instead of exiting the process (defers won’t run with os.Exit).Return the error to let Cobra handle it and ensure deferred cleanups execute.
- if err != nil { - logger.Err(err).Msg("nats failed to establish connection") - os.Exit(1) - } + if err != nil { + logger.Err(err).Msg("nats failed to establish connection") + return err + }
57-60: Return subscribe errors; avoid os.Exit in RunE.Preserves defers and aligns with the new error-propagation approach.
- if err != nil { - logger.Err(err).Msg("nats failed to subscribe") - os.Exit(1) - } + if err != nil { + logger.Err(err).Msg("nats failed to subscribe") + return err + }
52-53: Optionally drain the NATS connection during shutdown.Gracefully flushes pending messages prior to close.
- defer natsConn.Close() + defer func() { + _ = natsConn.Drain() + }()
64-70: Don’t log an error event on normal shutdown.Avoid emitting an error log when err is nil.
- g.Go(func() error { - err := service.NewValidator(service.NewPasswordSvc(cfg)).Process(ctx, mch) - - logger.Err(err).Msg("validator process") - return err - }) + g.Go(func() error { + err := service.NewValidator(service.NewPasswordSvc(cfg)).Process(ctx, mch) + if err != nil { + logger.Err(err).Msg("validator process") + } + return err + })
3-16: Treat delivery channel closure as graceful termination.Mirror context cancellation handling for dictionary.ErrDeliveryChannelClosed.
import ( "context" "errors" "os" "github.com/nats-io/nats.go" + "github.com/stockwayup/pass/dictionary" "github.com/stockwayup/pass/transport" "github.com/rs/zerolog" "github.com/spf13/cobra" "github.com/stockwayup/pass/conf" "github.com/stockwayup/pass/service" "golang.org/x/sync/errgroup" ) @@ - err = g.Wait() - if err == nil || errors.Is(err, context.Canceled) { + err = g.Wait() + if err == nil || + errors.Is(err, context.Canceled) || + errors.Is(err, dictionary.ErrDeliveryChannelClosed) { return nil } logger.Err(err).Msg("wait goroutines") return errAlso applies to: 72-79
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
.github/workflows/main.yml(4 hunks).golangci.yml(1 hunks)Dockerfile(2 hunks)Makefile(2 hunks)README.md(1 hunks)cmd/generate_consumer.go(4 hunks)cmd/validate_consumer.go(5 hunks)conf/conf.go(1 hunks)conf/config.github.json(0 hunks)conf/config.json(0 hunks)go.mod(1 hunks)main.go(1 hunks)service/generator.go(2 hunks)service/validator.go(2 hunks)transport/nats.go(1 hunks)
💤 Files with no reviewable changes (2)
- conf/config.json
- conf/config.github.json
🧰 Additional context used
🧬 Code Graph Analysis (2)
service/validator.go (5)
dictionary/errors.go (1)
ErrDeliveryChannelClosed(5-5)dictionary/key.go (1)
ID(5-5)transport/event/validate.go (1)
Validate(5-9)dictionary/queue.go (3)
TypeValidatedError(8-8)TypeValid(6-6)TypeInvalid(7-7)service/password.go (1)
Password(15-17)
service/generator.go (6)
dictionary/errors.go (1)
ErrDeliveryChannelClosed(5-5)dictionary/key.go (1)
ID(5-5)transport/event/generate.go (1)
Generate(5-7)dictionary/queue.go (2)
TypeGeneratedError(5-5)TypeGenerated(4-4)service/password.go (1)
Password(15-17)transport/event/generated.go (1)
Generated(5-8)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🪛 Checkov (3.2.334)
Dockerfile
[LOW] 1-24: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-24: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🔇 Additional comments (9)
.github/workflows/main.yml (3)
42-42: golangci-lint action version updated — compatible with Go 1.23.x.Looks good. Consider locking linter config drift by keeping the action version and the binary version aligned with your .golangci.yml changes in the same PR (which you’ve done).
52-54: Consistent Go version across jobs.Tests also run on Go 1.23.4 — consistent with lint job. LGTM.
25-26: Go 1.23.4 directive in go.mod confirmedgo.mod’s
godirective on line 3 is already set to1.23.4, matching the CI workflow. No further changes needed.transport/nats.go (1)
4-4: Import cleanup only — no functional changes.Reordering imports to keep stdlib first is fine. No logic changes in NewConnection. LGTM.
README.md (1)
3-3: Updated description reads clearer.Concise and accurate with current functionality. LGTM.
conf/conf.go (1)
17-18: Confirm NATS subject rename and update dependent services/configsThe default NATS subjects in conf/conf.go have been changed from
"validation"→"pass.validation""generation"→"pass.generation"This is a breaking change: any publishers or subscribers still using the old subjects will silently drop messages. Please ensure:
• All publisher code (in this repo or downstream services) uses the updated
cfg.Nats.Queues.Validationandcfg.Nats.Queues.Generationvalues.
• Any Helm charts, environment-variable overrides or secret stores referencing the old subjects are updated.
• Release notes and user documentation clearly call out the subject rename.Optional de-risking: Introduce a transient bridge that subscribes on the old subjects and republishes to the new ones for one release cycle. Let me know if you’d like a patch for this.
.golangci.yml (1)
3-8: Vendor mode for linters requires checked-in vendor/You’ve set modules-download-mode: vendor. If vendor/ is not committed or kept in sync, lint runs may fail or diverge from build behavior. See go.mod comment for a script to verify presence and freshness.
service/generator.go (2)
20-26: Receiver changed to value: non-breaking and fine for this typeSwitching Process to a value receiver is OK here: Generator contains only a pointer field, so copying is cheap, and method sets remain callable from both values and pointers. No functional concerns.
38-41: Graceful cancellation path added: LGTMHandling <-ctx.Done() ensures clean shutdown with proper log line from the defer.
| go generate ./... && \ | ||
| go build -o /bin/stockwayup | ||
| CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /bin/stockwayup | ||
|
|
||
| FROM alpine:3.20 |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Pinned msgp and trimmed build are good; consider reproducibility and build metadata.
- Nice pin to msgp@v1.3.0 and -trimpath -s -w. For reproducibility, consider -buildvcs=false to avoid embedding VCS info when not needed.
Proposed tweak within the changed lines:
- CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /bin/stockwayup
+ CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-s -w" -o /bin/stockwayup🤖 Prompt for AI Agents
In Dockerfile around lines 10 to 13, the current go build uses -trimpath -s -w
which strips debug info but still may embed VCS metadata; update the build flags
to include -buildvcs=false in the ldflags so VCS info is not recorded (e.g.,
change ldflags to "-s -w -buildvcs=false") to improve reproducibility and avoid
embedding repository metadata into the binary.
| go 1.23.4 | ||
|
|
||
| require ( | ||
| github.com/jinzhu/configor v1.2.2 | ||
| github.com/nats-io/nats.go v1.37.0 | ||
| github.com/rs/zerolog v1.33.0 | ||
| github.com/spf13/cobra v1.8.1 | ||
| github.com/nats-io/nats.go v1.44.0 | ||
| github.com/rs/zerolog v1.34.0 | ||
| github.com/spf13/cobra v1.9.1 | ||
| github.com/tinylib/msgp v1.3.0 | ||
| go.uber.org/automaxprocs v1.6.0 | ||
| golang.org/x/crypto v0.28.0 | ||
| golang.org/x/sync v0.8.0 | ||
| golang.org/x/crypto v0.41.0 | ||
| golang.org/x/sync v0.16.0 | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Align CI/tooling with Go 1.23.4 and verify vendoring after dependency bumps
- Go version bumped to 1.23.4; ensure CI (actions, Docker, Makefile) uses the same toolchain or a compatible one.
- Lints are configured for modules-download-mode: vendor; make sure vendor/ exists and is up-to-date with these new versions.
- Direct dep
github.com/tinylib/msgpis added; typically generated code imports it directly, so a blank import in main may be unnecessary (see main.go comment).
Run this quick check to validate repo state:
Also applies to: 16-31
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking vendor mode preconditions..."
if [ -d vendor ]; then
echo "vendor/ directory exists"
else
echo "ERROR: vendor/ directory not found but linters are set to vendor mode"; exit 1
fi
if [ -f vendor/modules.txt ]; then
echo "vendor/modules.txt present"
else
echo "ERROR: vendor/modules.txt missing; run 'go mod vendor'"; exit 1
fi
echo
echo "Checking CI/tooling references to Go version..."
rg -n -C2 -g '!vendor/**' -g '!**/node_modules/**' -P '(setup-go|golang|FROM golang|toolchain|go-version).*1\.23' || trueLength of output: 282
Ensure vendoring and align CI/tooling with Go 1.23.4
The repository is missing a vendor/ directory (linters are set to modules-download-mode=vendor), and CI/tooling does not reference Go 1.23.4. Please address the following before merging:
• Generate and commit your vendored dependencies:
go mod vendorVerify that vendor/ and vendor/modules.txt are present and up-to-date.
• Update all CI and build configurations to use Go 1.23.4 (or a compatible toolchain):
– .github/workflows/*.yml (actions/setup-go → 1.23.4)
– Dockerfile FROM golang:1.23.4
– Makefile or any scripts invoking go (ensure GO_VERSION=1.23.4)
• Review main.go’s blank import of github.com/tinylib/msgp. If generated code imports it directly, you can safely remove the redundant import.
🤖 Prompt for AI Agents
In go.mod around lines 3 to 14, the project declares Go 1.23.4 but is missing a
vendor/ tree and CI/tooling do not reference Go 1.23.4; run `go mod vendor` and
commit the generated vendor/ directory and vendor/modules.txt, update CI and
build configs to use Go 1.23.4 (update .github/workflows/* actions/setup-go
versions, Dockerfile base image to golang:1.23.4, Makefile or scripts setting
GO_VERSION), ensure linters are configured to use modules-download-mode=vendor
or adjust them to not require vendor, and remove the blank import of
github.com/tinylib/msgp from main.go if it’s unused by generated code.
| "github.com/rs/zerolog" | ||
| "github.com/rs/zerolog/log" | ||
| "github.com/stockwayup/pass/cmd" | ||
| _ "github.com/tinylib/msgp/msgp" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Drop the blank import of msgp if not strictly needed
Runtime usage of msgp typically comes from generated code that imports github.com/tinylib/msgp/msgp directly. A blank import in main adds init side effects and binary size without necessity.
[suggest_optional_refactor]
Apply:
- _ "github.com/tinylib/msgp/msgp"If you want to double-check that generated code already imports msgp, run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for msgp-generated files and imports..."
rg -n --type=go -C2 '_gen\.go$' transport || true
rg -n --type=go -C2 'github\.com/tinylib/msgp/msgp' || true
rg -n --type=go -C2 '(UnmarshalMsg|MarshalMsg)\(' transport || trueLength of output: 483
Drop the unused msgp blank import
No generated msgp code or direct msgp usage was found in the repo. You can safely remove the blank import in main.go to avoid extra init side-effects and binary size:
• main.go:9
- _ "github.com/tinylib/msgp/msgp"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ "github.com/tinylib/msgp/msgp" |
🤖 Prompt for AI Agents
In main.go around line 9, there is an unused blank import `_
"github.com/tinylib/msgp/msgp"` which should be removed; edit main.go to delete
that import line (and adjust the import block formatting if necessary) so the
build no longer includes the unnecessary init side-effects or increases binary
size.
| lint: goimports fmt | ||
| golangci-lint run --fix |
There was a problem hiding this comment.
🛠️ Refactor suggestion
lint target chaining is good; declare phony targets and ensure goimports availability.
- Add .PHONY to avoid collisions with files named like targets.
- goimports must be installed; consider a simple tools bootstrap.
Apply this diff within the changed lines:
-lint: goimports fmt
- golangci-lint run --fix
+lint: goimports fmt
+ golangci-lint run --fixAdd (outside the changed lines) near the top:
.PHONY: lint fmt goimports gen test build tools
tools:
go install golang.org/x/tools/cmd/goimports@latestOptionally make lint depend on tools:
lint: tools goimports fmt🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In Makefile around lines 1-2, add a .PHONY declaration and a tools bootstrap
plus a goimports target and make lint depend on tools/goimports/fmt: declare
.PHONY listing lint fmt goimports gen test build tools at the top of the file,
add a tools target that installs golang.org/x/tools/cmd/goimports@latest, add a
goimports target that runs goimports (e.g., writing changes back), and update
the lint target to depend on tools goimports fmt so goimports is ensured before
lint runs.
| if err := s.run(ctx, msg); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Don’t tear down the consumer on single-message failures.
Returning the first non-nil error from s.run stops the consumer. Prefer logging and continuing unless the context is canceled or the delivery channel is closed.
- if err := s.run(ctx, msg); err != nil {
- return err
- }
+ if err := s.run(ctx, msg); err != nil {
+ zerolog.Ctx(ctx).Err(err).Msg("message processing failed; continuing")
+ continue
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if err := s.run(ctx, msg); err != nil { | |
| return err | |
| } | |
| if err := s.run(ctx, msg); err != nil { | |
| zerolog.Ctx(ctx).Err(err).Msg("message processing failed; continuing") | |
| continue | |
| } |
🤖 Prompt for AI Agents
In service/validator.go around lines 34-37, the current code returns the first
non-nil error from s.run which tears down the consumer; instead, change the flow
to log the error and continue processing messages unless the context is canceled
(ctx.Done) or the delivery channel is closed. Specifically, replace the direct
return of err with logic that: if ctx.Err() != nil or the delivery channel is
closed, return the error to stop; otherwise log the error (and increment any
failure metric) and continue the loop so a single message failure does not stop
the consumer.
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (3)
.github/workflows/main.yml (2)
34-42: Pin golangci-lint to a reproducible version + configYou’ve bumped the action to v6 and version: v2.4.0. Good. Consider pinning the linter config via args or .golangci.yml only (avoid drift between local/CI). If .golangci.yml is authoritative, you can drop extra inputs here except version.
28-33: Pin msgp in CI to the same version as DockerfileThe generator tooling is installed with “go get -u -t” and “go install” (unpinned). This can cause nondeterminism and accidental go.mod churn. Dockerfile pins msgp@v1.3.0; CI should match.
Use a pinned install in both jobs:
- go get -u -t github.com/tinylib/msgp - go install github.com/tinylib/msgp + go install github.com/tinylib/msgp@v1.3.0 make genRepeat for the tests job’s “Generate” step.
Dockerfile (1)
13-23: Add a HEALTHCHECK to improve container observabilityStatic analysis suggests adding a HEALTHCHECK. Since the binary is a long-running process, expose a lightweight self-check if available, or a simple TCP check/exec probe.
Example:
FROM alpine:3.21 @@ USER www-data +HEALTHCHECK --interval=30s --timeout=5s --retries=3 CMD /bin/stockwayup --healthcheck || exit 1 CMD ["/bin/stockwayup"]Adjust the command/flags to match your binary’s capabilities.
♻️ Duplicate comments (14)
service/validator.go (5)
34-36: Don’t tear down the consumer on single-message failures; log and continueA per-message run error (often a transient NATS respond failure) shouldn’t stop the consumer loop.
- if err := s.run(ctx, msg); err != nil { - return err - } + if err := s.run(ctx, msg); err != nil { + zerolog.Ctx(ctx).Err(err).Msg("message processing failed; continuing") + continue + }
45-45: Unify header key usage with dictionary.IDAvoid string literals; use the canonical key source to reduce drift.
- msgID := msg.Header.Get("id") + msgID := msg.Header.Get(string(dictionary.ID))
57-67: Include headers on error replies (unmarshal path) for correlationRaw Respond drops headers; preserve correlation by replying with id/type headers.
- if _, err := in.UnmarshalMsg(msg.Data); err != nil { + if _, err := in.UnmarshalMsg(msg.Data); err != nil { zerolog.Ctx(ctx).Err(err).Msg("unmarshal msg") - if err := respond(msg, []byte(dictionary.TypeValidatedError)); err != nil { + errMsg := nats.NewMsg("") + if errMsg.Header == nil { + errMsg.Header = nats.Header{} + } + errMsg.Header.Set("id", msgID) + errMsg.Header.Set("type", dictionary.TypeValidatedError) + errMsg.Data = []byte(dictionary.TypeValidatedError) + + if err := respondMsg(msg, errMsg); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } - return nil + // non-critical: reply sent, continue processing next messages + return nil }
69-81: Include headers on error replies (validator error path) for correlationSame as above for the IsValid error branch.
- if err != nil { + if err != nil { zerolog.Ctx(ctx).Err(err).Msg("is valid") - if err := respond(msg, []byte(dictionary.TypeValidatedError)); err != nil { + errMsg := nats.NewMsg("") + if errMsg.Header == nil { + errMsg.Header = nats.Header{} + } + errMsg.Header.Set("id", msgID) + errMsg.Header.Set("type", dictionary.TypeValidatedError) + errMsg.Data = []byte(dictionary.TypeValidatedError) + + if err := respondMsg(msg, errMsg); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } // non-critical: reply sent, continue processing next messages return nil }
88-95: Initialize reply headers: LGTM; consider setting subject to msg.ReplyYou correctly guard Header against nil (avoids panic). Optionally set the Subject to msg.Reply for clarity (RespondMsg will route to Reply anyway).
- reply := nats.NewMsg("") + reply := nats.NewMsg(msg.Reply) if reply.Header == nil { reply.Header = nats.Header{} }service/generator.go (6)
34-36: Don’t stop the consumer on per-message failures; log and continueSame resilience concern as validator: treat per-message reply failures as non-fatal.
- if err := s.run(ctx, msg); err != nil { - return err - } + if err := s.run(ctx, msg); err != nil { + zerolog.Ctx(ctx).Err(err).Str("stage", "run").Msg("failed to process message; continuing") + continue + }
45-45: Unify header key usage with dictionary.IDAvoid magic strings; use dictionary.ID consistently.
- msgID := msg.Header.Get("id") + msgID := msg.Header.Get(string(dictionary.ID))
61-69: Include headers on error replies (unmarshal error) for consistent routingReplying with raw bytes drops headers; preserve id/type headers.
- if err := respond(msg, []byte(dictionary.TypeGeneratedError)); err != nil { + errReply := nats.NewMsg("") + if errReply.Header == nil { + errReply.Header = nats.Header{} + } + errReply.Header.Set("id", msgID) + errReply.Header.Set("type", dictionary.TypeGeneratedError) + errReply.Data = []byte(dictionary.TypeGeneratedError) + + if err := respondMsg(msg, errReply); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } // non-critical: reply sent, continue processing next messages return nil
75-81: Include headers on error replies (hashing error)Keep error reply semantics aligned across all error branches.
- if err := respond(msg, []byte(dictionary.TypeGeneratedError)); err != nil { + errReply := nats.NewMsg("") + if errReply.Header == nil { + errReply.Header = nats.Header{} + } + errReply.Header.Set("id", msgID) + errReply.Header.Set("type", dictionary.TypeGeneratedError) + errReply.Data = []byte(dictionary.TypeGeneratedError) + + if err := respondMsg(msg, errReply); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } return nil
90-98: Include headers on error replies (marshal error)Same consistency improvement for the marshal failure branch.
- if err := respond(msg, []byte(dictionary.TypeGeneratedError)); err != nil { + errReply := nats.NewMsg("") + if errReply.Header == nil { + errReply.Header = nats.Header{} + } + errReply.Header.Set("id", msgID) + errReply.Header.Set("type", dictionary.TypeGeneratedError) + errReply.Data = []byte(dictionary.TypeGeneratedError) + + if err := respondMsg(msg, errReply); err != nil { zerolog.Ctx(ctx).Err(err).Msg("nats queue respond") return err } // non-critical: reply sent, continue processing next messages return nil
100-107: Initialize reply headers: LGTM; optional: set subject to msg.ReplyHeader init prevents nil map panics. Consider setting Subject to msg.Reply for clarity (RespondMsg uses Reply regardless).
- reply := nats.NewMsg("") + reply := nats.NewMsg(msg.Reply) if reply.Header == nil { reply.Header = nats.Header{} }Dockerfile (1)
9-11: Reproducibility: pin tool install and avoid embedding VCS metadataGood pin of msgp and trimmed build. Consider explicitly setting GOFLAGS to empty with quotes and adding -buildvcs=false to keep builds reproducible and silence hadolint SC1007.
-RUN GOFLAGS= go install github.com/tinylib/msgp@v1.3.0 && \ +RUN GOFLAGS='' go install github.com/tinylib/msgp@v1.3.0 && \ go generate ./... && \ - CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o /bin/stockwayup + CGO_ENABLED=0 go build -trimpath -buildvcs=false -ldflags="-s -w" -o /bin/stockwayupMakefile (2)
1-2: Ensure goimports is available before lint runsIf contributors lack goimports locally, the target fails. Add a simple tools bootstrap and mark phony targets.
Within current lines, keep as-is; add outside these lines:
.PHONY: lint fmt goimports gen test build docker-build docker-push tools tools: go install golang.org/x/tools/cmd/goimports@latest lint: tools goimports fmt
17-24: Separate image name and tag to reduce drift with CIIMAGE includes the tag; splitting TAG eases bumping and matches the workflow’s pass-0.0.13 tag.
-IMAGE ?= soulgarden/swup:pass-0.0.13 +IMAGE ?= soulgarden/swup +TAG ?= pass-0.0.13 @@ -docker-build: - docker build . -t $(IMAGE) --platform $(PLATFORM) +docker-build: + docker build . -t $(IMAGE):$(TAG) --platform $(PLATFORM) @@ -docker-push: - docker push $(IMAGE) +docker-push: + docker push $(IMAGE):$(TAG)Add the TAG variable definition outside these lines if needed (shown above).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
.github/workflows/main.yml(4 hunks)Dockerfile(2 hunks)Makefile(1 hunks)go.mod(1 hunks)service/generator.go(2 hunks)service/generator_run_test.go(1 hunks)service/nats_stub.go(1 hunks)service/validator.go(2 hunks)service/validator_run_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
service/generator_run_test.go (2)
conf/conf.go (1)
Config(10-27)service/password.go (2)
Password(15-17)NewPasswordSvc(19-21)
service/validator_run_test.go (5)
service/password.go (2)
Password(15-17)NewPasswordSvc(19-21)conf/conf.go (1)
Config(10-27)service/validator.go (1)
Validator(12-14)transport/event/validate.go (1)
Validate(5-9)dictionary/queue.go (3)
TypeValid(6-6)TypeInvalid(7-7)TypeValidatedError(8-8)
service/validator.go (5)
dictionary/errors.go (1)
ErrDeliveryChannelClosed(5-5)dictionary/key.go (1)
ID(5-5)transport/event/validate.go (1)
Validate(5-9)dictionary/queue.go (3)
TypeValidatedError(8-8)TypeValid(6-6)TypeInvalid(7-7)service/password.go (1)
Password(15-17)
service/generator.go (3)
dictionary/errors.go (1)
ErrDeliveryChannelClosed(5-5)dictionary/key.go (1)
ID(5-5)service/password.go (1)
Password(15-17)
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
[warning] 1-1: Missing required phony target "test"
(minphony)
[warning] 27-27: Target "build" should be declared PHONY.
(phonydeclared)
🪛 Checkov (3.2.334)
Dockerfile
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
[LOW] 1-23: Ensure that HEALTHCHECK instructions have been added to container images
(CKV_DOCKER_2)
🪛 Hadolint (2.12.0)
Dockerfile
[warning] 9-9: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (6)
service/nats_stub.go (1)
5-9: Solid, minimal test seam for NATS repliesUsing package-level function variables to stub NATS reply behavior is a pragmatic approach and integrates cleanly with the tests.
service/generator_run_test.go (1)
13-64: Well-structured success-path test; good assertionsCapturing respond vs respondMsg to assert the correct path and verifying headers plus payload structure is spot-on.
.github/workflows/main.yml (2)
61-65: RabbitMQ references removed—tests and CI workflow clean
No occurrences of RabbitMQ/AMQP or RABBITMQ_ env vars found in code, tests, or.github/workflows/main.yml.
25-25: Confirm Go 1.25.0 support in CI and Docker imageBefore merging, please verify that:
- actions/setup-go@v5 can install Go 1.25.0 as specified in
• .github/workflows/main.yml: lines 23–27 & 49–54- the Docker base image
golang:1.25.0-alpine3.21exists on Docker Hub (Dockerfile:1)- your go.mod declares
go 1.25.0(go.mod:3)This will help avoid unexpected CI failures due to an unavailable Go version.
go.mod (1)
3-3: Go 1.25.0 alignment verified
- go.mod specifies
go 1.25.0- Dockerfile uses
FROM golang:1.25.0-alpine3.21- No other toolchain references (
GOTOOLCHAIN,actions/setup-go,go-version) found in the repoPlease ensure:
- All developers have Go 1.25.0 installed locally
- Any CI workflows (e.g. GitHub Actions using actions/setup-go) are pinned to 1.25.0
- golangci-lint (or other linters) in CI support Go 1.25.0
Dockerfile (1)
1-1: Verify Docker base image tags exist before mergingEnsure that both
golang:1.25.0-alpine3.21andalpine:3.21are published on Docker Hub. Locally, you can run:# Option A: Pull the images (will error if a tag doesn’t exist) docker pull golang:1.25.0-alpine3.21 docker pull alpine:3.21 # Option B: Inspect the manifest (faster, no layers downloaded) docker manifest inspect golang:1.25.0-alpine3.21 docker manifest inspect alpine:3.21If either command fails, update the
FROMlines in your Dockerfile to use the closest available tags.— Applies to Dockerfile lines 1 and 13.
| tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13 | ||
| cache-from: type=registry,ref=${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-latest |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid duplicating the image tag; centralize once
The tag pass-0.0.13 is hardcoded here and in the Makefile. Prefer a single source (env/Makefile) to prevent drift during bumps.
Apply this diff to wire tags through env and reuse across steps:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13
+ tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ vars.SWUP_TAG || 'pass-0.0.13' }}And likewise for the push step:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13
+ tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ vars.SWUP_TAG || 'pass-0.0.13' }}Optionally define SWUP_TAG as an org/repo-level Actions variable.
Also applies to: 98-100
| github.com/nats-io/nats.go v1.44.0 | ||
| github.com/rs/zerolog v1.34.0 | ||
| github.com/spf13/cobra v1.9.1 | ||
| github.com/tinylib/msgp v1.3.0 | ||
| go.uber.org/automaxprocs v1.6.0 | ||
| golang.org/x/crypto v0.28.0 | ||
| golang.org/x/sync v0.8.0 | ||
| golang.org/x/crypto v0.41.0 | ||
| golang.org/x/sync v0.16.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Module upgrades look sane; run tidy to normalize graph
After dependency bumps, ensure the graph is tidy and vendor (if used) is updated.
Also applies to: 16-31
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "go mod tidy (dry guidance): ensure no diff after tidy"
go mod tidy
git status --porcelain
echo "If vendor mode is used anywhere, ensure vendor/ is updated:"
rg -n -P '(-mod=vendor|GOFLAGS=.*-mod=vendor|modules-download-mode\s*:\s*vendor)' || trueLength of output: 1829
Run go mod tidy and commit the resulting changes
After bumping dependencies, running go mod tidy modified both go.mod and go.sum. Please:
- Run
go mod tidylocally. - Commit the updated
go.modandgo.sum. - If your project uses vendoring, update the
vendor/directory accordingly.
(Also applies to lines 16–31 in go.mod.)
🤖 Prompt for AI Agents
In go.mod around lines 7–13 (and also apply to lines 16–31), running `go mod
tidy` after the dependency bumps is required: run `go mod tidy` locally to
update both go.mod and go.sum, review the diffs, commit the updated go.mod and
go.sum, and if the repo uses vendoring run `go mod vendor` (or your vendoring
tool) and commit the updated vendor/ tree as well.
| github.com/nats-io/nats.go v1.44.0 | ||
| github.com/rs/zerolog v1.34.0 | ||
| github.com/spf13/cobra v1.9.1 | ||
| github.com/tinylib/msgp v1.3.0 |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
msgp as a direct dependency: consider tools pattern
If msgp isn’t imported at runtime, keep it out of production deps by using a tools.go with //go:build tools and listing msgp via import for tooling only.
[suggest_optional_refactor]
Example tools.go (outside current diff):
//go:build tools
package tools
import _ "github.com/tinylib/msgp"Then ensure you install with “go install github.com/tinylib/msgp@v1.3.0” in CI/Docker and remove msgp from direct require if not used by runtime code.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verify runtime import of msgp (should be none unless intentional):"
rg -n -C2 -P '^\s*import\s*(\(|")' --type go | rg -n 'tinylib/msgp' -n -C2 || trueLength of output: 244
Move msgp into a tools-only dependency
We’ve confirmed there are no runtime imports of github.com/tinylib/msgp. To keep your production module lean, you can move it into a tools.go file under a build tag.
• In go.mod (line 10): remove
- github.com/tinylib/msgp v1.3.0• Create (or update) tools/tools.go with:
//go:build tools
package tools
import _ "github.com/tinylib/msgp"• In CI/Docker, install the generator explicitly:
go install github.com/tinylib/msgp@v1.3.0🤖 Prompt for AI Agents
In go.mod around line 10, remove the runtime dependency entry
"github.com/tinylib/msgp v1.3.0" and instead add a tools-only file under
tools/tools.go with a build tag that imports the package for tooling (use a
blank import) so it’s not included in production builds; then update CI/Docker
commands to explicitly install the generator with "go install
github.com/tinylib/msgp@v1.3.0" so code generation remains available in
pipelines.
| goimports: | ||
| goimports -w . |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
goimports target: declare phony and optional bootstrap
Mark as phony and consider making goimports depend on tools for first-time setup.
Apply outside these lines (paired with previous comment):
.PHONY: goimports
goimports: tools
goimports -w .🤖 Prompt for AI Agents
In Makefile around lines 7 to 8, the goimports target is not declared phony and
does not depend on the tools bootstrap, so add a .PHONY declaration for
goimports and make the target depend on tools (i.e., add a .PHONY: goimports
line and change the target to "goimports: tools" so goimports runs after the
tools bootstrap).
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Declare build target as phony
checkmake flagged build as missing .PHONY. Add it to avoid filename collisions.
Outside these lines:
.PHONY: build🧰 Tools
🪛 checkmake (0.2.2)
[warning] 27-27: Target "build" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
In Makefile around lines 26 to 27, the "build" target is not declared phony
which can cause filename collisions; add a .PHONY declaration for build (e.g.,
add a line declaring .PHONY: build) outside the target definition so the
Makefile treats "build" as a phony target rather than a file.
| func newPassSvc() *Password { | ||
| cfg := &conf.Config{} | ||
| cfg.Password.Time = 1 | ||
| cfg.Password.Memory = 65536 | ||
| cfg.Password.Threads = 1 | ||
| cfg.Password.KeyLen = 32 | ||
| return NewPasswordSvc(cfg) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
DRY up password service test setup
newPassSvc is handy; consider moving it to a shared test helper (e.g., service/test_helpers_test.go) and reuse it across generator tests to avoid duplication.
Also applies to: 23-36, 60-67
🤖 Prompt for AI Agents
In service/validator_run_test.go around lines 13-20 (and similarly apply to
ranges 23-36 and 60-67), the newPassSvc helper is duplicated; extract this
function into a shared test helper file (e.g., service/test_helpers_test.go)
with package test or same package depending on visibility, export or keep
unexported as needed, then replace the duplicated implementations in
validator_run_test.go and the other referenced ranges with calls to the shared
newPassSvc helper to DRY up setup and reuse across generator tests.
| var captured *nats.Msg | ||
| oldRespondMsg := respondMsg | ||
| respondMsg = func(_ *nats.Msg, reply *nats.Msg) error { captured = reply; return nil } | ||
| defer func() { respondMsg = oldRespondMsg }() |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer t.Cleanup for restoring global stubs
t.Cleanup guarantees restoration even if subtests are introduced or control flow changes. It also reads cleaner than defer for global state resets.
- defer func() { respondMsg = oldRespondMsg }()
+ t.Cleanup(func() { respondMsg = oldRespondMsg })- defer func() { respondMsg = oldRespondMsg }()
+ t.Cleanup(func() { respondMsg = oldRespondMsg })- defer func() { respond = oldRespond }()
+ t.Cleanup(func() { respond = oldRespond })Also applies to: 76-76, 101-101
🤖 Prompt for AI Agents
In service/validator_run_test.go around lines 41, 76, and 101, the test
currently restores the global stub using defer (e.g., defer func() { respondMsg
= oldRespondMsg }()), which can fail to run reliably with subtests or changed
control flow; replace each defer-based restoration with t.Cleanup(func() {
respondMsg = oldRespondMsg }) so the original global is always restored after
the test completes and remove the corresponding defer statements.
| if captured == nil { | ||
| t.Fatalf("no reply") | ||
| } | ||
| if got := captured.Header.Get("type"); got != dictionary.TypeValid { | ||
| t.Errorf("type=%q", got) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assert correlation ID is preserved in the reply
You already assert the type; also assert the id header to lock correlation behavior.
if captured == nil {
t.Fatalf("no reply")
}
+ if gotID := captured.Header.Get("id"); gotID != "v1" {
+ t.Errorf("id=%q", gotID)
+ }
if got := captured.Header.Get("type"); got != dictionary.TypeValid {
t.Errorf("type=%q", got)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if captured == nil { | |
| t.Fatalf("no reply") | |
| } | |
| if got := captured.Header.Get("type"); got != dictionary.TypeValid { | |
| t.Errorf("type=%q", got) | |
| } | |
| if captured == nil { | |
| t.Fatalf("no reply") | |
| } | |
| if gotID := captured.Header.Get("id"); gotID != "v1" { | |
| t.Errorf("id=%q", gotID) | |
| } | |
| if got := captured.Header.Get("type"); got != dictionary.TypeValid { | |
| t.Errorf("type=%q", got) | |
| } |
🤖 Prompt for AI Agents
In service/validator_run_test.go around lines 50 to 55, the test asserts the
reply type but does not assert the correlation id header; add an assertion that
captured.Header.Get("id") equals the same correlation id value used when sending
the request (use the existing variable that set the request's id), e.g. fetch
the expected id from that variable and call t.Errorf or t.Fatalf if they differ
so the test enforces that the reply preserves the correlation id.
| func TestValidator_run_Invalid(t *testing.T) { | ||
|
|
||
| svc := newPassSvc() | ||
| v := Validator{password: svc} | ||
|
|
||
| hash, salt, err := svc.HashPassword(context.Background(), []byte("secret")) | ||
| if err != nil { | ||
| t.Fatalf("hash: %v", err) | ||
| } | ||
|
|
||
| in := event.Validate{Input: []byte("wrong"), Password: hash, Salt: salt} | ||
| data, err := in.MarshalMsg(nil) | ||
| if err != nil { | ||
| t.Fatalf("marshal: %v", err) | ||
| } | ||
|
|
||
| var captured *nats.Msg | ||
| oldRespondMsg := respondMsg | ||
| respondMsg = func(_ *nats.Msg, reply *nats.Msg) error { captured = reply; return nil } | ||
| defer func() { respondMsg = oldRespondMsg }() | ||
|
|
||
| msg := &nats.Msg{Header: nats.Header{}} | ||
| msg.Header.Set("id", "v2") | ||
| msg.Data = data | ||
|
|
||
| if err := v.run(context.Background(), msg); err != nil { | ||
| t.Fatalf("run: %v", err) | ||
| } | ||
| if captured == nil { | ||
| t.Fatalf("no reply") | ||
| } | ||
| if got := captured.Header.Get("type"); got != dictionary.TypeInvalid { | ||
| t.Errorf("type=%q", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mirror the id assertion on the invalid path for symmetry
Same as the valid test, validate that correlation ID is preserved.
if captured == nil {
t.Fatalf("no reply")
}
+ if gotID := captured.Header.Get("id"); gotID != "v2" {
+ t.Errorf("id=%q", gotID)
+ }
if got := captured.Header.Get("type"); got != dictionary.TypeInvalid {
t.Errorf("type=%q", got)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestValidator_run_Invalid(t *testing.T) { | |
| svc := newPassSvc() | |
| v := Validator{password: svc} | |
| hash, salt, err := svc.HashPassword(context.Background(), []byte("secret")) | |
| if err != nil { | |
| t.Fatalf("hash: %v", err) | |
| } | |
| in := event.Validate{Input: []byte("wrong"), Password: hash, Salt: salt} | |
| data, err := in.MarshalMsg(nil) | |
| if err != nil { | |
| t.Fatalf("marshal: %v", err) | |
| } | |
| var captured *nats.Msg | |
| oldRespondMsg := respondMsg | |
| respondMsg = func(_ *nats.Msg, reply *nats.Msg) error { captured = reply; return nil } | |
| defer func() { respondMsg = oldRespondMsg }() | |
| msg := &nats.Msg{Header: nats.Header{}} | |
| msg.Header.Set("id", "v2") | |
| msg.Data = data | |
| if err := v.run(context.Background(), msg); err != nil { | |
| t.Fatalf("run: %v", err) | |
| } | |
| if captured == nil { | |
| t.Fatalf("no reply") | |
| } | |
| if got := captured.Header.Get("type"); got != dictionary.TypeInvalid { | |
| t.Errorf("type=%q", got) | |
| } | |
| } | |
| if captured == nil { | |
| t.Fatalf("no reply") | |
| } | |
| if gotID := captured.Header.Get("id"); gotID != "v2" { | |
| t.Errorf("id=%q", gotID) | |
| } | |
| if got := captured.Header.Get("type"); got != dictionary.TypeInvalid { | |
| t.Errorf("type=%q", got) | |
| } |
🤖 Prompt for AI Agents
In service/validator_run_test.go around lines 58 to 92, the test for the invalid
path currently checks that a reply was produced and that its "type" header
equals dictionary.TypeInvalid but doesn't assert the correlation ID; mirror the
valid test by asserting captured.Header.Get("id") == "v2". After the nil check
for captured (right before or after the type assertion) add an assertion that
compares the captured header "id" to "v2" and call t.Errorf (or t.Fatalf) with a
clear message if it doesn't match so the correlation ID preservation is tested
on the invalid path as well.
| func TestValidator_run_UnmarshalError(t *testing.T) { | ||
|
|
||
| v := Validator{password: newPassSvc()} | ||
|
|
||
| var dataSent []byte | ||
| oldRespond := respond | ||
| respond = func(_ *nats.Msg, data []byte) error { dataSent = append([]byte(nil), data...); return nil } | ||
| defer func() { respond = oldRespond }() | ||
|
|
||
| msg := &nats.Msg{Header: nats.Header{}} | ||
| msg.Header.Set("id", "v3") | ||
| msg.Data = []byte("bad") | ||
|
|
||
| if err := v.run(context.Background(), msg); err != nil { | ||
| t.Fatalf("run: %v", err) | ||
| } | ||
| if string(dataSent) != dictionary.TypeValidatedError { | ||
| t.Errorf("expected %q, got %q", dictionary.TypeValidatedError, string(dataSent)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Optional: capture error replies via respondMsg to verify headers too
If error replies start including headers (recommended), update this test to capture via respondMsg and assert the error type header.
- var dataSent []byte
- oldRespond := respond
- respond = func(_ *nats.Msg, data []byte) error { dataSent = append([]byte(nil), data...); return nil }
- defer func() { respond = oldRespond }()
+ var captured *nats.Msg
+ oldRespondMsg := respondMsg
+ respondMsg = func(_ *nats.Msg, reply *nats.Msg) error { captured = reply; return nil }
+ t.Cleanup(func() { respondMsg = oldRespondMsg })
@@
if err := v.run(context.Background(), msg); err != nil {
t.Fatalf("run: %v", err)
}
- if string(dataSent) != dictionary.TypeValidatedError {
- t.Errorf("expected %q, got %q", dictionary.TypeValidatedError, string(dataSent))
- }
+ if captured == nil {
+ t.Fatalf("no reply")
+ }
+ if got := captured.Header.Get("type"); got != dictionary.TypeValidatedError {
+ t.Errorf("type=%q", got)
+ }
+ if string(captured.Data) != dictionary.TypeValidatedError {
+ t.Errorf("expected %q, got %q", dictionary.TypeValidatedError, string(captured.Data))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestValidator_run_UnmarshalError(t *testing.T) { | |
| v := Validator{password: newPassSvc()} | |
| var dataSent []byte | |
| oldRespond := respond | |
| respond = func(_ *nats.Msg, data []byte) error { dataSent = append([]byte(nil), data...); return nil } | |
| defer func() { respond = oldRespond }() | |
| msg := &nats.Msg{Header: nats.Header{}} | |
| msg.Header.Set("id", "v3") | |
| msg.Data = []byte("bad") | |
| if err := v.run(context.Background(), msg); err != nil { | |
| t.Fatalf("run: %v", err) | |
| } | |
| if string(dataSent) != dictionary.TypeValidatedError { | |
| t.Errorf("expected %q, got %q", dictionary.TypeValidatedError, string(dataSent)) | |
| } | |
| } | |
| func TestValidator_run_UnmarshalError(t *testing.T) { | |
| v := Validator{password: newPassSvc()} | |
| var captured *nats.Msg | |
| oldRespondMsg := respondMsg | |
| respondMsg = func(_ *nats.Msg, reply *nats.Msg) error { | |
| captured = reply | |
| return nil | |
| } | |
| t.Cleanup(func() { respondMsg = oldRespondMsg }) | |
| msg := &nats.Msg{Header: nats.Header{}} | |
| msg.Header.Set("id", "v3") | |
| msg.Data = []byte("bad") | |
| if err := v.run(context.Background(), msg); err != nil { | |
| t.Fatalf("run: %v", err) | |
| } | |
| if captured == nil { | |
| t.Fatalf("no reply") | |
| } | |
| if got := captured.Header.Get("type"); got != dictionary.TypeValidatedError { | |
| t.Errorf("type=%q", got) | |
| } | |
| if string(captured.Data) != dictionary.TypeValidatedError { | |
| t.Errorf("expected %q, got %q", dictionary.TypeValidatedError, string(captured.Data)) | |
| } | |
| } |
🤖 Prompt for AI Agents
In service/validator_run_test.go around lines 94 to 113, the test captures error
replies via the respond hook which only records message data; update the test to
use respondMsg instead so you can inspect headers as well: replace the respond
override with a respondMsg override that stores the full *nats.Msg (copy if
needed) and defer-restores the original, then after v.run completes assert both
that the body equals dictionary.TypeValidatedError and that the returned message
Header.Get("type" or the agreed error header key) equals
dictionary.TypeValidatedError (or the expected header value).
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
.github/workflows/main.yml (2)
28-33: Avoid mutating go.mod in CI and fix msgp install (use module@version).
go get -u -twill attempt to modify go.mod/go.sum. CI should not change the repo.go install github.com/tinylib/msgpwithout@versionfails for remote installs in recent Go versions.Apply this diff:
- name: Generate run: |- - go get -u -t github.com/tinylib/msgp - go install github.com/tinylib/msgp + go install github.com/tinylib/msgp@latest make gen
55-59: Repeat: don’t mutate go.mod in CI; install msgp with an explicit version.Same issue as the lint job’s Generate step.
Apply this diff:
- name: Generate run: |- - go get -u -t github.com/tinylib/msgp - go install github.com/tinylib/msgp + go install github.com/tinylib/msgp@latest make gen
♻️ Duplicate comments (1)
.github/workflows/main.yml (1)
88-100: Avoid hardcoding the image tag twice; centralize to a single variable.Prevents drift on future bumps.
Apply this diff to both build paths:
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13 + tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ vars.SWUP_TAG || 'pass-0.0.13' }}Optionally, define SWUP_TAG at org/repo level Actions variables, or set once in the workflow
env:block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/main.yml(4 hunks)
🔇 Additional comments (1)
.github/workflows/main.yml (1)
35-43: No changes needed – golangci-lint v2.4.0 is valid
According to the official release notes, v2.4.0 is the latest golangci-lint version (released August 14, 2025). You can keep theversion: v2.4.0pin in.github/workflows/main.yml.Likely an incorrect or invalid review comment.
| go-version: 1.25.0 | ||
| cache: false |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Prefer minor-only Go spec to auto-receive latest patch (reduce churn).
Pinning to 1.25.0 forces updates in CI on every patch release. Let setup-go resolve to the latest 1.25.x instead.
Apply this diff:
- go-version: 1.25.0
+ go-version: '1.25'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go-version: 1.25.0 | |
| cache: false | |
| go-version: '1.25' | |
| cache: false |
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 25 to 26: the workflow pins Go to an
exact patch version "1.25.0" which forces CI updates on every patch; change the
go-version value to the minor-only spec "1.25" so setup-go will automatically
use the latest 1.25.x patch release (leave cache: false as-is).
| go-version: 1.25.0 | ||
| cache: false | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Mirror Go version pinning change in tests job.
Keep both jobs consistent and avoid patch churn.
Apply this diff:
- go-version: 1.25.0
+ go-version: '1.25'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go-version: 1.25.0 | |
| cache: false | |
| go-version: '1.25' | |
| cache: false |
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 52 to 54: the tests job does not mirror
the go-version and cache settings used elsewhere, causing unnecessary patch
churn; update the tests job configuration to set go-version: 1.25.0 and cache:
false to match the other job so both jobs remain consistent.
Remove unsupported parameters (skip-pkg-cache, skip-build-cache) that were deprecated in golangci-lint-action v7.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
.github/workflows/main.yml (4)
25-26: Pin Go by minor or via go.mod to avoid patch churn and drift.Use a minor-only spec or go-version-file to stay current on 1.25.x and keep CI aligned with go.mod.
Apply one of:
- go-version: 1.25.0 + go-version: '1.25'Or:
- go-version: 1.25.0 + go-version-file: 'go.mod'
50-52: Mirror Go pinning change here as well (minor or go.mod).Keep both jobs consistent and reduce update churn.
Apply one of:
- go-version: 1.25.0 + go-version: '1.25'Or:
- go-version: 1.25.0 + go-version-file: 'go.mod'
86-87: Avoid hardcoding image tag; centralize a single source of truth.Use an Actions variable (e.g., SWUP_TAG) with a sane default to prevent drift with Makefile/Dockerfile.
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13 + tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ vars.SWUP_TAG || 'pass-0.0.13' }}
96-96: Deduplicate tag definition in push step as well.Same rationale as the build step; keep tag configurable via env/vars.
- tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:pass-0.0.13 + tags: ${{ secrets.DOCKER_HUB_USERNAME }}/swup:${{ vars.SWUP_TAG || 'pass-0.0.13' }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.github/workflows/main.yml(4 hunks)
| uses: golangci/golangci-lint-action@v7 | ||
| with: | ||
| only-new-issues: true | ||
| skip-cache: true | ||
| skip-pkg-cache: true | ||
| skip-build-cache: true | ||
| working-directory: . | ||
| version: v1.56.2 | ||
| version: v2.4.0 | ||
| tests: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
golangci-lint version ‘v2.4.0’ is invalid; action will fail to install.
golangci-lint releases are v1.x; there’s no v2.x. Either set a valid v1.x or use latest. Also confirm whether skip-cache is still a supported input in v7.
Proposed fix:
- uses: golangci/golangci-lint-action@v7
+ uses: golangci/golangci-lint-action@v7
with:
only-new-issues: true
- skip-cache: true
working-directory: .
- version: v2.4.0
+ version: latestOptionally re-add caching control if v7 still supports it:
+ skip-cache: trueVerify the latest valid linter version and supported inputs:
🌐 Web query:
What is the latest release version of golangci-lint, and does golangci/golangci-lint-action@v7 support the "skip-cache" input?
💡 Result:
-
Latest golangci-lint release: v2.1.6 (released May 4, 2025). (github.com)
-
Does golangci/golangci-lint-action@v7 support the skip-cache input? Yes — the v7 action metadata (action.yml) includes a skip-cache input (default: false). (github.com)
I checked the project release and the action repository as of Aug 17, 2025. Would you like the exact snippet from action.yml or an example workflow showing skip-cache in use?
Citations:
- 1: https://github.com/golangci/golangci-lint/releases?utm_source=chatgpt.com
- 2: https://github.com/golangci/golangci-lint-action/blob/v7/action.yml
golangci-lint version v2.4.0 is invalid – update to the latest v2.x release (v2.1.6) or use latest
Please adjust the workflow to use a valid golangci-lint release and retain the supported skip-cache input:
- File: .github/workflows/main.yml
• Around line 38: change theversion:input fromv2.4.0to a valid release
• You may pin tov2.1.6for reproducibility or uselatestto always pick the newest patch
Proposed diff:
uses: golangci/golangci-lint-action@v7
with:
only-new-issues: true
skip-cache: true
working-directory: .
- version: v2.4.0
+ version: v2.1.6 # latest v2.x release (or `latest`)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 35 to 41: the workflow uses an invalid
golangci-lint `version: v2.4.0`; update the `version:` input to a valid release
(e.g., pin to `v2.1.6` for reproducibility or set to `latest` to always use the
newest patch) and keep the existing `skip-cache: true` and `only-new-issues:
true` inputs unchanged so lint action inputs remain supported.
- Change version from numeric 2 to string "2" - Replace enable-all with linters.default: all - Move skip-files to linters.exclusions.paths - Move exclude-rules to linters.exclusions.rules
- Exclude generated files from most strict linters - Disable problematic linters globally for existing codebase - Fix CI lint failures while maintaining code quality
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.golangci.yml(1 hunks)
🔇 Additional comments (6)
.golangci.yml (6)
3-5: Run config looks sane (timeout + vendor mode).5m is a reasonable upper bound for larger codebases, and vendor mode helps ensure deterministic dependency resolution in CI.
29-33: Noise risk: funlen/dupl on all code and again in test rule.You list
funlenandduplin the global set and again in the test path rule. If the goal is to only allow these two in tests, ensure the per-path mechanism actually constrains the linter set; otherwise this duplication is redundant and tests still inherit all linters.Once the per-path mechanism is confirmed, either:
- remove them from the global list, or
- remove the test-specific rule if it’s not constraining.
17-20: Clarify golangci-lint per-path rule intentThe
text:field only applies underissues.exclude-rules(it’s used to suppress matching messages), so your current block under alinters:section will be ignored. Please confirm which behavior you actually want:
- Suppress errcheck’s “Error return value… not checked” in generated tests → move this rule under
issues.exclude-rules.- Enable only errcheck for those files → use the v2 per-path enable mechanism or isolate into a separate CI job.
Location:
- .golangci.yml (lines 17–20)
Suggested suppression config:
issues: exclude-rules: - path: ^transport/event/.*_gen_test\.go$ linters: - errcheck text: "Error return value of .* is not checked"
7-9: No changes needed forlinters.default: all
golangci-lint v2’s configuration useslinters.default: all(orstandard/fast/none) to enable linters by default, replacing the oldenable-allknob. You can safely keepdefault: allin your.golangci.yml.Likely an incorrect or invalid review comment.
9-21: No issues found—per-path linter exclusions syntax is valid
Thelinters.exclusionsblock in v2 supports bothpaths:andrules:in parallel. Your configuration correctly uses:
linters.exclusions.pathsto exclude by file patternlinters.exclusions.rulesfor per-linter, regex‐based disablesThis matches the v2 docs (see migration guide and false-positives section), so the block will be recognized and applied as intended. You can ignore the previous suggestion.
Likely an incorrect or invalid review comment.
1-1: Confirm CI uses a v2-aware golangci-lintI didn’t find any GitHub Actions workflows under .github/workflows to verify the linter invocation. Please ensure whichever CI you’re using (GitHub Actions, CircleCI, GitLab CI, Travis CI, Jenkins, etc.) is running golangci-lint v2.x or newer so it correctly reads
version: "2"and doesn’t silently fall back.Check your CI configuration files, for example:
- .github/workflows/*.yml
- .gitlab-ci.yml
- .circleci/config.yml
- .travis.yml
- Jenkinsfile
| linters: | ||
| - gci | ||
| - gofumpt | ||
| - exhaustivestruct | ||
| - exhaustruct | ||
| - wrapcheck | ||
| - tagliatelle |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Duplicate/overlapping linters: pick one of exhaustivestruct vs exhaustruct.
Both report “uninitialized struct fields”; running both creates duplicate findings and churn. Prefer the maintained exhaustruct.
- gofumpt
- - exhaustivestruct
- exhaustruct📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - path: .* | |
| linters: | |
| - gci | |
| - gofumpt | |
| - exhaustivestruct | |
| - exhaustruct | |
| - wrapcheck | |
| - path: .* | |
| linters: | |
| - gci | |
| - gofumpt | |
| - exhaustruct | |
| - wrapcheck |
🤖 Prompt for AI Agents
In .golangci.yml around lines 21 to 27 the configuration enables both
exhaustivestruct and exhaustruct which duplicate the same “uninitialized struct
fields” checks; remove one to avoid duplicate findings — keep the maintained
exhaustruct linter and delete the exhaustivestruct entry from the linters list
so only exhaustruct remains.
| - gofumpt | ||
| - exhaustivestruct |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Stabilize formatter/import linters with settings to avoid churn.
Without settings, gci and gofumpt defaults may not match your existing style/import groups.
Outside this block, consider:
linters-settings:
gofumpt:
extra: true
gci:
sections:
- standard
- default
- prefix(your/module/path)
skip-generated: true🤖 Prompt for AI Agents
.golangci.yml around lines 23-24: gci and gofumpt linters are enabled but lack
configuration which causes formatter/import churn; add a linters-settings block
that sets gofumpt.extra: true and configures gci.sections (standard, default,
and prefix(<your/module/path>)) and skip-generated: true so the tools produce
stable, project-specific formatting; replace <your/module/path> with the
repository module path.
This parameter may be causing CI failures with v7 action
Use go install to install golangci-lint v2.4.0 and run via make lint to avoid GitHub Action compatibility issues
Change modules-download-mode from vendor to readonly to avoid vendor directory sync issues in CI
Let golangci-lint use default module mode instead of vendor/readonly
Direct make lint fails due to missing goimports in CI environment
# Conflicts: # .github/workflows/main.yml # Dockerfile # Makefile # go.mod # go.sum # service/generator.go # service/validator.go
1 participant
Summary by CodeRabbit
Refactor
Tests
Chores