Merged
Conversation
🤖 Pull request artifacts
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a new "logs" CLI command with RFC3339 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Validator
participant Client
participant Server
participant Renderer
User->>CLI: run "smsgate logs --from T1 --to T2"
CLI->>Validator: parse RFC3339 timestamps and validate range
alt invalid range
Validator-->>CLI: error (From > To)
CLI-->>User: stderr (params error)
else valid range
Validator-->>CLI: ok
CLI->>Client: GetLogs(ctx, from, to)
Client->>Server: HTTP GET /logs?from=...&to=...
Server-->>Client: [LogEntry...]
Client-->>CLI: log entries
CLI->>Renderer: Logs(entries)
Renderer-->>CLI: formatted output (JSON or text)
CLI-->>User: stdout (rendered logs)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 Tip CodeRabbit can use your project's `golangci-lint` configuration to improve the quality of Go code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
133-137:⚠️ Potential issue | 🟡 MinorUpdate command-group count text to match the list.
Line 133 says “two main groups of commands,” but the list now has three groups (including Logs on Line 137).
✏️ Proposed doc fix
-The CLI offers two main groups of commands: +The CLI offers three main command groups:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 133 - 137, Update the header text that currently reads "two main groups of commands" to accurately reflect the list containing three groups; locate the sentence in README.md that precedes the bullet list (the phrase "The CLI offers two main groups of commands:") and change it to say "three main groups of commands" or reword to "The CLI offers the following command groups:" so it matches the bullet items "Messages", "Webhooks", and "Logs".
🧹 Nitpick comments (1)
internal/commands/logs/logs.go (1)
44-53: Consolidate duplicated timestamp presence checks.
from/tonil validation is repeated in bothlogsBeforeandlogsAction. Consider centralizing this to one helper to reduce drift risk.Also applies to: 58-63
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/logs/logs.go` around lines 44 - 53, Extract the repeated nil-check logic for c.Timestamp("from") and c.Timestamp("to") into a small helper (e.g., requireTimestamps or validateTimestamps) that returns the two *time.Time values and an error (using cli.Exit with codes.ParamsError) and replace the duplicated presence checks in logsBefore and logsAction with calls to that helper; keep the existing from.After(*to) ordering check inside logsBefore after calling the helper so behavior is unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/core/output/text.go`:
- Around line 4-5: Replace the use of fmt.Sprintf for integer formatting to fix
the perfsprint linter: change the call fmt.Sprintf("%d", entry.ID) to
strconv.FormatUint(uint64(entry.ID), 10), and update the imports by removing
"fmt" and adding "strconv" while keeping "strings"; look for the occurrence of
fmt.Sprintf and the symbol entry.ID in this file (text.go) to make the
replacement.
---
Outside diff comments:
In `@README.md`:
- Around line 133-137: Update the header text that currently reads "two main
groups of commands" to accurately reflect the list containing three groups;
locate the sentence in README.md that precedes the bullet list (the phrase "The
CLI offers two main groups of commands:") and change it to say "three main
groups of commands" or reword to "The CLI offers the following command groups:"
so it matches the bullet items "Messages", "Webhooks", and "Logs".
---
Nitpick comments:
In `@internal/commands/logs/logs.go`:
- Around line 44-53: Extract the repeated nil-check logic for
c.Timestamp("from") and c.Timestamp("to") into a small helper (e.g.,
requireTimestamps or validateTimestamps) that returns the two *time.Time values
and an error (using cli.Exit with codes.ParamsError) and replace the duplicated
presence checks in logsBefore and logsAction with calls to that helper; keep the
existing from.After(*to) ordering check inside logsBefore after calling the
helper so behavior is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60ff7f32-7b1b-4011-b848-0b312c78f65e
📒 Files selected for processing (7)
README.mdcmd/smsgate/smsgate.gointernal/commands/logs/logs.gointernal/core/output/json.gointernal/core/output/output.gointernal/core/output/text.gotests/e2e/logs_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/commands/logs/logs.go (1)
51-54: Consider adding nil checks for client and renderer.
metadata.GetClientandmetadata.GetRendererreturnnilif the metadata is missing or the type assertion fails. While the parentBeforehook should always set these, defensive nil checks would prevent cryptic panics if the initialization order ever changes.🛡️ Proposed defensive checks
client := metadata.GetClient(c.App.Metadata) renderer := metadata.GetRenderer(c.App.Metadata) + + if client == nil || renderer == nil { + return cli.Exit("internal error: client or renderer not initialized", codes.InternalError) + } res, err := client.GetLogs(c.Context, *from, *to)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/logs/logs.go` around lines 51 - 54, Add defensive nil checks after calling metadata.GetClient and metadata.GetRenderer: verify the local variables client and renderer are not nil before using them (specifically before calling client.GetLogs and any renderer methods). If either is nil, return an explicit error (or print a clear message and exit) so the command fails with a helpful message instead of panicking; reference the metadata.GetClient/metadata.GetRenderer calls and the client variable used in client.GetLogs to locate where to insert the checks.internal/flags/period.go (1)
9-26: Default timestamps are captured at command registration time, not execution time.
time.Now()is evaluated whenPeriod()is called during CLI setup, not when the command is executed. For typical CLI usage (short-lived processes), this is fine. However, if the CLI were ever used in a long-running context or the command definitions are cached, the defaults could become stale.If this becomes a concern, consider using
DefaultTextfor documentation and computing defaults lazily in the action/before hook when values are nil.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/flags/period.go` around lines 9 - 26, The Period() function sets default timestamp Values using time.Now() at registration which can become stale; change to not set Value eagerly—remove the cli.NewTimestamp(time.Now()) assignments from the &cli.TimestampFlag{...} and instead set DefaultText (e.g., "24h ago" / "now") for help, then compute and populate the actual defaults lazily in the command's Before or Action hook by checking the flags returned by cli.Context (e.g., via ctx.IsSet/ctx.Timestamp) and, if unset/nil, setting the from/to times with time.Now() and ctx.Set or by handling nil values in the command logic. Ensure you update references to Period(), TimestampFlag.Value, and the command Before/Action that consumes these flags.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/logs/logs.go`:
- Around line 51-54: Add defensive nil checks after calling metadata.GetClient
and metadata.GetRenderer: verify the local variables client and renderer are not
nil before using them (specifically before calling client.GetLogs and any
renderer methods). If either is nil, return an explicit error (or print a clear
message and exit) so the command fails with a helpful message instead of
panicking; reference the metadata.GetClient/metadata.GetRenderer calls and the
client variable used in client.GetLogs to locate where to insert the checks.
In `@internal/flags/period.go`:
- Around line 9-26: The Period() function sets default timestamp Values using
time.Now() at registration which can become stale; change to not set Value
eagerly—remove the cli.NewTimestamp(time.Now()) assignments from the
&cli.TimestampFlag{...} and instead set DefaultText (e.g., "24h ago" / "now")
for help, then compute and populate the actual defaults lazily in the command's
Before or Action hook by checking the flags returned by cli.Context (e.g., via
ctx.IsSet/ctx.Timestamp) and, if unset/nil, setting the from/to times with
time.Now() and ctx.Set or by handling nil values in the command logic. Ensure
you update references to Period(), TimestampFlag.Value, and the command
Before/Action that consumes these flags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3b3b8047-01ad-4fad-a418-50fbb666fe58
📒 Files selected for processing (4)
dev-docsinternal/commands/logs/logs.gointernal/core/output/text.gointernal/flags/period.go
✅ Files skipped from review due to trivial changes (1)
- dev-docs
capcom6
force-pushed
the
codex/add-logs-management-command-to-cli
branch
from
63f7497 to
82d5292
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/commands/logs/logs.go`:
- Around line 51-54: The code currently dereferences
metadata.GetClient(c.App.Metadata) and metadata.GetRenderer(c.App.Metadata) into
client and renderer and then calls client.GetLogs(...) and
renderer.RenderLogs(...), which can panic if metadata wiring failed; add nil
checks immediately after calling metadata.GetClient and metadata.GetRenderer
and, if either is nil, return a clear CLI error (or propagate an error) instead
of proceeding to call GetLogs or RenderLogs. Specifically, after obtaining
client and renderer, validate client != nil before calling client.GetLogs and
validate renderer != nil before using renderer (e.g., RenderLogs), and return an
appropriate error message indicating missing metadata wiring.
In `@tests/e2e/logs_test.go`:
- Around line 49-55: The tests currently call exec.Command("./smsgate", ...)
which depends on running from tests/e2e; update TestMain to export the built
binary path (e.g. set SMSGATE_BIN to the actual output path such as
tests/e2e/smsgate or compute it dynamically) and change the test to read binPath
:= os.Getenv("SMSGATE_BIN") (with a clear fallback or fail fast if empty) and
use binPath in exec.Command instead of "./smsgate"; also fix TestMain cleanup to
os.Remove the same computed path (not "smsgate") so the built artifact is
removed correctly and the tests no longer rely on working directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ae88f208-bac4-44a9-b17b-460039c94135
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
README.mdcmd/smsgate/smsgate.godev-docsgo.modinternal/commands/logs/logs.gointernal/core/client/client.gointernal/core/output/json.gointernal/core/output/output.gointernal/core/output/text.gointernal/flags/period.gotests/e2e/logs_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/smsgate/smsgate.go
- internal/core/output/output.go
- dev-docs
- internal/core/output/json.go
- README.md
capcom6
force-pushed
the
codex/add-logs-management-command-to-cli
branch
from
527a82f to
ca094ba
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (3)
README.md (1)
199-239: Consider adding example output for logs command.The "Output formats" section shows example outputs for message status, but doesn't include examples of what the logs output looks like in text, JSON, and raw formats. This would help users understand the logs output structure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 199 - 239, The "Output formats" section currently shows examples for message status but omits the logs command; add example log outputs for Text, JSON, and Raw formats under the same "Output formats" heading showing typical fields (timestamp, level, message, context/metadata) and at least one multi-line text log line, one JSON object with keys like "timestamp", "level", "message", "service" and optional "metadata", and the compact raw JSON string version; update the README examples near the existing message status samples so users can see how the logs command output maps across Text, JSON, and Raw formats.tests/e2e/messages_test.go (1)
20-23: Consider extracting the binary path retrieval to a helper function.The same
os.Getenv("SMSGATE_BIN")+ fatal check pattern is repeated in every test function across multiple test files. This could be simplified with a helper:func requireBinPath(t *testing.T) string { t.Helper() binPath := os.Getenv("SMSGATE_BIN") if binPath == "" { t.Fatal("SMSGATE_BIN environment variable is not set") } return binPath }Then each test would simply call
binPath := requireBinPath(t).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/messages_test.go` around lines 20 - 23, Extract the repeated os.Getenv("SMSGATE_BIN") + fatal check into a test helper: add a function requireBinPath(t *testing.T) that calls t.Helper(), reads os.Getenv("SMSGATE_BIN"), calls t.Fatal if empty, and returns the path; then replace direct uses of os.Getenv("SMSGATE_BIN") and the t.Fatal check in tests (e.g., messages_test.go and other e2e test files) with binPath := requireBinPath(t) to centralize the logic and reduce duplication.internal/commands/logs/logs.go (1)
46-49: Consider passing validated period through context to avoid re-parsing.
ParsePeriodFlagsis called twice—once inlogsBefore(line 32) for validation and again here. While harmless, you could store the validated period in the context's local storage duringBeforeand retrieve it inActionto avoid redundant parsing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/commands/logs/logs.go` around lines 46 - 49, logsBefore currently calls ParsePeriodFlags for validation and the Action handler calls ParsePeriodFlags again; instead have logsBefore call ParsePeriodFlags once, store the validated period value in the CLI/context local storage (e.g., under a unique key), and in the Action handler retrieve that stored period rather than re-parsing; update references to the local variable named period in the Action to use the retrieved value and keep ParsePeriodFlags referenced only from logsBefore to avoid redundant work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/commands/logs/logs.go`:
- Around line 46-49: logsBefore currently calls ParsePeriodFlags for validation
and the Action handler calls ParsePeriodFlags again; instead have logsBefore
call ParsePeriodFlags once, store the validated period value in the CLI/context
local storage (e.g., under a unique key), and in the Action handler retrieve
that stored period rather than re-parsing; update references to the local
variable named period in the Action to use the retrieved value and keep
ParsePeriodFlags referenced only from logsBefore to avoid redundant work.
In `@README.md`:
- Around line 199-239: The "Output formats" section currently shows examples for
message status but omits the logs command; add example log outputs for Text,
JSON, and Raw formats under the same "Output formats" heading showing typical
fields (timestamp, level, message, context/metadata) and at least one multi-line
text log line, one JSON object with keys like "timestamp", "level", "message",
"service" and optional "metadata", and the compact raw JSON string version;
update the README examples near the existing message status samples so users can
see how the logs command output maps across Text, JSON, and Raw formats.
In `@tests/e2e/messages_test.go`:
- Around line 20-23: Extract the repeated os.Getenv("SMSGATE_BIN") + fatal check
into a test helper: add a function requireBinPath(t *testing.T) that calls
t.Helper(), reads os.Getenv("SMSGATE_BIN"), calls t.Fatal if empty, and returns
the path; then replace direct uses of os.Getenv("SMSGATE_BIN") and the t.Fatal
check in tests (e.g., messages_test.go and other e2e test files) with binPath :=
requireBinPath(t) to centralize the logic and reduce duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c01595d-8c76-49be-a0a5-b18f19991d97
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
README.mdcmd/smsgate/smsgate.godev-docsgo.modinternal/commands/logs/logs.gointernal/core/client/client.gointernal/core/output/json.gointernal/core/output/output.gointernal/core/output/text.gointernal/flags/period.gotests/e2e/general_test.gotests/e2e/logs_test.gotests/e2e/main_test.gotests/e2e/messages_test.gotests/e2e/webhooks_test.go
✅ Files skipped from review due to trivial changes (1)
- dev-docs
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/e2e/logs_test.go
- internal/core/client/client.go
- internal/core/output/text.go
- go.mod
- internal/flags/period.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/e2e/messages_test.go (1)
199-202: Prefer the sharedRequireBinPathhelper in all tests.These repeated
os.Getenv("SMSGATE_BIN")checks duplicate logic already centralized ine2e/testutils. Reusing the helper keeps behavior consistent and avoids drift.♻️ Suggested refactor pattern
- binPath := os.Getenv("SMSGATE_BIN") - if binPath == "" { - t.Fatal("SMSGATE_BIN environment variable is not set") - } + binPath := testutils.RequireBinPath(t)Also applies to: 301-304, 331-334, 396-399, 445-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/messages_test.go` around lines 199 - 202, Replace the manual os.Getenv("SMSGATE_BIN") checks and t.Fatal calls in this test with the shared helper RequireBinPath to avoid duplicated logic; locate the occurrences that set binPath := os.Getenv("SMSGATE_BIN") and replace them by calling RequireBinPath(t) (and use its return value as the bin path) so the test reuses the centralized behavior in e2e/testutils instead of duplicating environment validation.tests/e2e/webhooks_test.go (1)
116-119: Usetestutils.RequireBinPath(t)consistently across this file.The repeated manual env-var checks can be replaced with the shared helper to reduce duplication and keep setup behavior uniform.
♻️ Suggested refactor pattern
- binPath := os.Getenv("SMSGATE_BIN") - if binPath == "" { - t.Fatal("SMSGATE_BIN environment variable is not set") - } + binPath := testutils.RequireBinPath(t)Also applies to: 193-196, 270-273, 352-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/webhooks_test.go` around lines 116 - 119, Replace the manual env-var checks that set binPath via os.Getenv("SMSGATE_BIN") followed by t.Fatal with a single call to testutils.RequireBinPath(t) that returns the required path; e.g., change occurrences that declare binPath and call t.Fatal into binPath := testutils.RequireBinPath(t). Update all similar blocks in this file (the other spots where binPath is set) so they consistently use testutils.RequireBinPath(t) instead of duplicating the getenv+t.Fatal pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/messages_test.go`:
- Around line 199-202: Replace the manual os.Getenv("SMSGATE_BIN") checks and
t.Fatal calls in this test with the shared helper RequireBinPath to avoid
duplicated logic; locate the occurrences that set binPath :=
os.Getenv("SMSGATE_BIN") and replace them by calling RequireBinPath(t) (and use
its return value as the bin path) so the test reuses the centralized behavior in
e2e/testutils instead of duplicating environment validation.
In `@tests/e2e/webhooks_test.go`:
- Around line 116-119: Replace the manual env-var checks that set binPath via
os.Getenv("SMSGATE_BIN") followed by t.Fatal with a single call to
testutils.RequireBinPath(t) that returns the required path; e.g., change
occurrences that declare binPath and call t.Fatal into binPath :=
testutils.RequireBinPath(t). Update all similar blocks in this file (the other
spots where binPath is set) so they consistently use testutils.RequireBinPath(t)
instead of duplicating the getenv+t.Fatal pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94388111-c6e3-4812-85c1-caef021ac8bb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
README.mdcmd/smsgate/smsgate.godev-docsgo.modinternal/commands/logs/logs.gointernal/core/client/client.gointernal/core/output/json.gointernal/core/output/output.gointernal/core/output/text.gointernal/flags/period.gotests/e2e/general_test.gotests/e2e/logs_test.gotests/e2e/main_test.gotests/e2e/messages_test.gotests/e2e/testutils/testutils.gotests/e2e/webhooks_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/core/output/json.go
- go.mod
- cmd/smsgate/smsgate.go
- internal/core/output/output.go
- internal/core/client/client.go
- dev-docs
- internal/commands/logs/logs.go
- internal/flags/period.go
- internal/core/output/text.go
capcom6
force-pushed
the
codex/add-logs-management-command-to-cli
branch
from
3c6bb30 to
caa5a15
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
GetLogscapability from the client library in the CLI so users can retrieve server logs for a given time range.--from/--toflags and validation to make queries reproducible and safe.Description
logscommand (internal/commands/logs/logs.go) which accepts--fromand--toRFC3339timestamps, validates thatfrom <= to, and callsclient.GetLogs(...).cmd/smsgate/smsgate.go) so it appears alongsidemessagesandwebhooks.Logs([]smsgateway.LogEntry)and implement it for text and JSON/raw formats (internal/core/output/output.go,internal/core/output/text.go,internal/core/output/json.go).tests/e2e/logs_test.go) and update README usage examples to document thelogscommand.Testing
go test ./...which completed successfully for the module packages.go test -run TestLogsinsidetests/e2e, which passed.Codex Task
Summary by CodeRabbit
New Features
Documentation
Tests