Conversation
WalkthroughUpdated Go/tooling and container base images, added a Docker host configuration section to the README, and replaced the logging implementation and API across the codebase (new SlogLogger, changed Logger interface and Context logging), with corresponding test updates and minor Docker image tag bump. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
14-24:⚠️ Potential issue | 🟠 MajorRun the final image as non-root user.
The runtime stage has no
USER, soofeliaruns as root. That is a security hardening gap and is also flagged by static analysis.🔒 Suggested hardening patch
FROM alpine:3.23 # this label is required to identify container with ofelia running LABEL ofelia.service=true LABEL ofelia.enabled=true -RUN apk --no-cache add ca-certificates tini tzdata +RUN apk --no-cache add ca-certificates tini tzdata \ + && addgroup -S ofelia \ + && adduser -S -G ofelia ofelia COPY --from=builder /go/bin/ofelia /usr/bin/ofelia +USER ofelia ENTRYPOINT ["/sbin/tini", "/usr/bin/ofelia"]If you mount
/var/run/docker.sock, ensure runtime group permissions are handled (for example viagroup_addmatching socket GID).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 14 - 24, The image launches ofelia as root; add a non-root runtime user and set USER before ENTRYPOINT so ofelia runs unprivileged—create a minimal user/group (e.g. ofelia user), ensure binary ownership and any required runtime dirs are chowned to that user (refer to the copied binary /usr/bin/ofelia and ENTRYPOINT), and document/handle socket access by allowing group membership or advising group_add for the Docker socket GID when mounting /var/run/docker.sock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Dockerfile`:
- Around line 14-24: The image launches ofelia as root; add a non-root runtime
user and set USER before ENTRYPOINT so ofelia runs unprivileged—create a minimal
user/group (e.g. ofelia user), ensure binary ownership and any required runtime
dirs are chowned to that user (refer to the copied binary /usr/bin/ofelia and
ENTRYPOINT), and document/handle socket access by allowing group membership or
advising group_add for the Docker socket GID when mounting /var/run/docker.sock.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fc9fff3-224f-4b7b-9e21-248acb876272
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
DockerfileREADME.mdgo.modintegration/test-run-exec/docker-compose.yml
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cli/docker_handler.go (1)
118-118: Keep the original error in fallback log context.Line 118 logs the fallback action but omits
err, which makes root-cause diagnosis harder.🛠️ Suggested tweak
- c.logger.Debug("Failed to extract ofelia's container ID. Trying with container hostname instead...") + c.logger.Debug("Failed to extract ofelia's container ID. Trying with container hostname instead...", "error", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/docker_handler.go` at line 118, The fallback debug log "Failed to extract ofelia's container ID. Trying with container hostname instead..." omits the original error; update the logging call in the Docker handler (the method in cli/docker_handler.go where c.logger.Debug is invoked) to include the original error variable (err) in the log context/message so the fallback log contains the root-cause error information (e.g., include err as a field or append err.Error() to the message).middlewares/common_test.go (1)
67-70: Consider aligning parameter names and types with the Logger interface.The
TestLoggermethods use(format string, args ...interface{})but theLoggerinterface incore/common.go:226-231defines(str string, args ...any). Whileinterface{}andanyare type aliases and this compiles correctly, matching the interface signature exactly improves consistency.♻️ Suggested alignment with interface
-func (*TestLogger) Debug(format string, args ...interface{}) {} -func (*TestLogger) Error(format string, args ...interface{}) {} -func (*TestLogger) Info(format string, args ...interface{}) {} -func (*TestLogger) Warning(format string, args ...interface{}) {} +func (*TestLogger) Debug(str string, args ...any) {} +func (*TestLogger) Error(str string, args ...any) {} +func (*TestLogger) Info(str string, args ...any) {} +func (*TestLogger) Warning(str string, args ...any) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middlewares/common_test.go` around lines 67 - 70, Update the TestLogger method signatures to exactly match the Logger interface by changing each method (Debug, Error, Info, Warning) to accept parameters named "str string, args ...any" instead of "format string, args ...interface{}"; this aligns parameter names and uses the same type alias (any) as in core/common.go's Logger interface for consistency.cli/config_test.go (1)
22-25: Consider renamingformatparameter tomsgorstrfor clarity.The parameter is named
formatbut the new logging API uses structured key-value pairs rather than printf-style formatting. Renaming tomsgorstr(matching theLoggerinterface incore/common.go) would better reflect the actual usage pattern.♻️ Suggested rename
-func (*TestLogger) Debug(format string, args ...interface{}) {} -func (*TestLogger) Error(format string, args ...interface{}) {} -func (*TestLogger) Info(format string, args ...interface{}) {} -func (*TestLogger) Warning(format string, args ...interface{}) {} +func (*TestLogger) Debug(msg string, args ...interface{}) {} +func (*TestLogger) Error(msg string, args ...interface{}) {} +func (*TestLogger) Info(msg string, args ...interface{}) {} +func (*TestLogger) Warning(msg string, args ...interface{}) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/config_test.go` around lines 22 - 25, Rename the parameter named `format` to `msg` (or `str`) in the TestLogger methods to match the structured logging API and the `Logger` interface in core/common.go: update the method signatures for Debug, Error, Info, and Warning on type `*TestLogger` so they accept `msg string, args ...interface{}` (or `str string, ...`) instead of `format string, ...`, and adjust any internal references to this parameter accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/cron_utils.go`:
- Around line 12-18: The Info method signature uses variadic ...any which breaks
implementation of robfig/cron v3 Logger; update CronUtils.Info to use the same
variadic type as CronUtils.Error (i.e., ...interface{}) so both Info and Error
match the cron.Logger interface expected by cron.WithLogger; modify the function
signature for CronUtils.Info accordingly while keeping its body unchanged.
In `@middlewares/slack.go`:
- Around line 64-67: The logs are currently emitting the sensitive Slack webhook
(m.SlackWebhook) via ctx.Logger.Error in the error paths; remove the webhook
from the log payload or replace it with a redacted/masked value before logging
to avoid secret exposure. Update the calls in the error handling branches (the
ctx.Logger.Error usages that reference m.SlackWebhook and r.Status/r.StatusCode)
to omit the "webhook" key or substitute a safe string like "<redacted webhook>"
or a masked snippet, ensuring no raw webhook URL appears in logs.
- Around line 62-67: The HTTP response from http.PostForm in
middlewares/slack.go is never closed which leaks connections; update the block
where r, err := http.PostForm(m.SlackWebhook, values) is handled so that when
err == nil you immediately ensure the body is closed (e.g., defer r.Body.Close()
or read io.Copy(io.Discard, r.Body) and then r.Body.Close()) before any returns
or logging; modify the handler around the PostForm call (referencing variables
r, err, m.SlackWebhook and ctx.Logger) to always close r.Body in both the 200
and non-200 response paths.
---
Nitpick comments:
In `@cli/config_test.go`:
- Around line 22-25: Rename the parameter named `format` to `msg` (or `str`) in
the TestLogger methods to match the structured logging API and the `Logger`
interface in core/common.go: update the method signatures for Debug, Error,
Info, and Warning on type `*TestLogger` so they accept `msg string, args
...interface{}` (or `str string, ...`) instead of `format string, ...`, and
adjust any internal references to this parameter accordingly.
In `@cli/docker_handler.go`:
- Line 118: The fallback debug log "Failed to extract ofelia's container ID.
Trying with container hostname instead..." omits the original error; update the
logging call in the Docker handler (the method in cli/docker_handler.go where
c.logger.Debug is invoked) to include the original error variable (err) in the
log context/message so the fallback log contains the root-cause error
information (e.g., include err as a field or append err.Error() to the message).
In `@middlewares/common_test.go`:
- Around line 67-70: Update the TestLogger method signatures to exactly match
the Logger interface by changing each method (Debug, Error, Info, Warning) to
accept parameters named "str string, args ...any" instead of "format string,
args ...interface{}"; this aligns parameter names and uses the same type alias
(any) as in core/common.go's Logger interface for consistency.
🪄 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: d96bfc98-709f-4838-b39d-236e71ee3781
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (21)
README.mdcli/config.gocli/config_test.gocli/daemon.gocli/docker_handler.gocli/validate.gocore/common.gocore/common_test.gocore/cron_utils.gocore/logger.gocore/runjob.gocore/runjob_test.gocore/runservice.gocore/runservice_test.gocore/scheduler.gogo.modmiddlewares/common_test.gomiddlewares/mail.gomiddlewares/save.gomiddlewares/slack.goofelia.go
✅ Files skipped from review due to trivial changes (3)
- middlewares/save.go
- README.md
- go.mod
| func (c *CronUtils) Info(msg string, keysAndValues ...any) { | ||
| c.Logger.Debug("cron update", append(keysAndValues, "cron", msg)...) | ||
| } | ||
|
|
||
| func (c *CronUtils) Error(err error, msg string, keysAndValues ...interface{}) { | ||
| c.Logger.Errorf("%v", formatKeysAndValues(append([]interface{}{"cron", msg, "error", err}, keysAndValues...)...)) | ||
| c.Logger.Error("cron error", append(keysAndValues, "cron", msg, "error", err)...) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
robfig cron v3 Logger interface signature
💡 Result:
The Logger interface in robfig/cron v3 is a subset of the go-logr/logr interface, with two methods: Info(msg string, keysAndValues ...interface{}) and Error(err error, msg string, keysAndValues ...interface{}). It is used by the Cron struct for logging events like start, schedule, wake, run, added, stop, removed. DefaultLogger is the default implementation. Usage example: cron.New(cron.WithLogger(cron.VerbosePrintfLogger(log.New(os.Stdout, "cron: ", log.LstdFlags)))) It complies with https://github.com/go-logr/logr project for structured logging.
Citations:
- 1: https://pkg.go.dev/github.com/robfig/cron/v3
- 2: https://go.pact.im/doc/use/github.com/robfig/cron/v3..Cron.logger%5E2686a.html
- 3: https://go.pact.im/doc/src/github.com/robfig/cron/v3/cron.go.html
- 4: https://go.pact.im/doc/src/github.com/robfig/cron/v3/chain.go.html
- 5: https://go.pact.im/doc/imp/github.com/robfig/cron/v3.Logger.html
- 6: Logger interface robfig/cron#202
🏁 Script executed:
# Check core/scheduler.go around line 29 to confirm CronUtils usage
fd core scheduler.go --exec head -50 {} \; | cat -nRepository: flywp/ofelia
Length of output: 155
🏁 Script executed:
# Search for cron.WithLogger usage to confirm CronUtils is passed there
rg "WithLogger" --type go -A 2 -B 2Repository: flywp/ofelia
Length of output: 286
🏁 Script executed:
# Check if there are any compilation or type errors related to CronUtils
rg "CronUtils" --type go -A 3 -B 1Repository: flywp/ofelia
Length of output: 1029
Fix Info method signature to match cron v3 Logger interface.
The CronUtils.Info method at line 12 uses ...any while the Error method uses ...interface{}. Since CronUtils implements the robfig/cron v3 Logger interface (confirmed by usage in core/scheduler.go:29 with cron.WithLogger(cronUtils)), both methods must use ...interface{} per the interface specification.
Change line 12 from ...any to ...interface{}:
Fix for interface compliance
-func (c *CronUtils) Info(msg string, keysAndValues ...any) {
+func (c *CronUtils) Info(msg string, keysAndValues ...interface{}) {
c.Logger.Debug("cron update", append(keysAndValues, "cron", msg)...)
}
func (c *CronUtils) Error(err error, msg string, keysAndValues ...interface{}) {
c.Logger.Error("cron error", append(keysAndValues, "cron", msg, "error", err)...)
}📝 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 (c *CronUtils) Info(msg string, keysAndValues ...any) { | |
| c.Logger.Debug("cron update", append(keysAndValues, "cron", msg)...) | |
| } | |
| func (c *CronUtils) Error(err error, msg string, keysAndValues ...interface{}) { | |
| c.Logger.Errorf("%v", formatKeysAndValues(append([]interface{}{"cron", msg, "error", err}, keysAndValues...)...)) | |
| c.Logger.Error("cron error", append(keysAndValues, "cron", msg, "error", err)...) | |
| } | |
| func (c *CronUtils) Info(msg string, keysAndValues ...interface{}) { | |
| c.Logger.Debug("cron update", append(keysAndValues, "cron", msg)...) | |
| } | |
| func (c *CronUtils) Error(err error, msg string, keysAndValues ...interface{}) { | |
| c.Logger.Error("cron error", append(keysAndValues, "cron", msg, "error", err)...) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/cron_utils.go` around lines 12 - 18, The Info method signature uses
variadic ...any which breaks implementation of robfig/cron v3 Logger; update
CronUtils.Info to use the same variadic type as CronUtils.Error (i.e.,
...interface{}) so both Info and Error match the cron.Logger interface expected
by cron.WithLogger; modify the function signature for CronUtils.Info accordingly
while keeping its body unchanged.
| r, err := http.PostForm(m.SlackWebhook, values) | ||
| if err != nil { | ||
| ctx.Logger.Errorf("Slack error calling %q error: %q", m.SlackWebhook, err) | ||
| ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err) | ||
| } else if r.StatusCode != 200 { | ||
| ctx.Logger.Errorf("Slack error non-200 status code calling %q", m.SlackWebhook) | ||
| ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify HTTP response bodies from PostForm are closed in Go code.
# Expected: each PostForm assignment to *http.Response has a corresponding Body.Close in-path.
rg -nP --type=go '(\w+)\s*,\s*err\s*:=\s*http\.PostForm\(' -C4
rg -nP --type=go '\.Body\.Close\s*\(' -C2Repository: flywp/ofelia
Length of output: 686
Close the HTTP response body after PostForm to avoid connection leaks.
The response body from http.PostForm() must be closed to return the connection to the pool. Currently, the code does not close r.Body in any execution path, even on non-200 responses.
♻️ Suggested fix
r, err := http.PostForm(m.SlackWebhook, values)
if err != nil {
ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err)
- } else if r.StatusCode != 200 {
+ return
+ }
+ defer r.Body.Close()
+
+ if r.StatusCode != 200 {
ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status)
}📝 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.
| r, err := http.PostForm(m.SlackWebhook, values) | |
| if err != nil { | |
| ctx.Logger.Errorf("Slack error calling %q error: %q", m.SlackWebhook, err) | |
| ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err) | |
| } else if r.StatusCode != 200 { | |
| ctx.Logger.Errorf("Slack error non-200 status code calling %q", m.SlackWebhook) | |
| ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status) | |
| } | |
| r, err := http.PostForm(m.SlackWebhook, values) | |
| if err != nil { | |
| ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err) | |
| return | |
| } | |
| defer r.Body.Close() | |
| if r.StatusCode != 200 { | |
| ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middlewares/slack.go` around lines 62 - 67, The HTTP response from
http.PostForm in middlewares/slack.go is never closed which leaks connections;
update the block where r, err := http.PostForm(m.SlackWebhook, values) is
handled so that when err == nil you immediately ensure the body is closed (e.g.,
defer r.Body.Close() or read io.Copy(io.Discard, r.Body) and then
r.Body.Close()) before any returns or logging; modify the handler around the
PostForm call (referencing variables r, err, m.SlackWebhook and ctx.Logger) to
always close r.Body in both the 200 and non-200 response paths.
| ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err) | ||
| } else if r.StatusCode != 200 { | ||
| ctx.Logger.Errorf("Slack error non-200 status code calling %q", m.SlackWebhook) | ||
| ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status) | ||
| } |
There was a problem hiding this comment.
Do not log Slack webhook URLs (sensitive secret exposure).
Line 64 and Line 66 include m.SlackWebhook in logs. Webhook URLs are credentials and should be redacted/omitted.
🔐 Suggested fix
- ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err)
+ ctx.Logger.Error("Failed to call slack", "error", err)
} else if r.StatusCode != 200 {
- ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status)
+ ctx.Logger.Error("Slack responded with non-200 status", "status", r.Status)
}📝 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.
| ctx.Logger.Error("Failed to call slack", "webhook", m.SlackWebhook, "error", err) | |
| } else if r.StatusCode != 200 { | |
| ctx.Logger.Errorf("Slack error non-200 status code calling %q", m.SlackWebhook) | |
| ctx.Logger.Error("Slack responded with non-200 status", "webhook", m.SlackWebhook, "status", r.Status) | |
| } | |
| ctx.Logger.Error("Failed to call slack", "error", err) | |
| } else if r.StatusCode != 200 { | |
| ctx.Logger.Error("Slack responded with non-200 status", "status", r.Status) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middlewares/slack.go` around lines 64 - 67, The logs are currently emitting
the sensitive Slack webhook (m.SlackWebhook) via ctx.Logger.Error in the error
paths; remove the webhook from the log payload or replace it with a
redacted/masked value before logging to avoid secret exposure. Update the calls
in the error handling branches (the ctx.Logger.Error usages that reference
m.SlackWebhook and r.Status/r.StatusCode) to omit the "webhook" key or
substitute a safe string like "<redacted webhook>" or a masked snippet, ensuring
no raw webhook URL appears in logs.
3 participants
Putting it in Draft Mode due to upstream build failure
Summary by CodeRabbit
Documentation
Chores
Refactor