Skip to content

deps: remove lfshook in favor of local fileHook#2464

Open
kotakanbe wants to merge 8 commits intomasterfrom
diet-lfshook
Open

deps: remove lfshook in favor of local fileHook#2464
kotakanbe wants to merge 8 commits intomasterfrom
diet-lfshook

Conversation

@kotakanbe
Copy link
Member

@kotakanbe kotakanbe commented Mar 16, 2026

Why (motivation for removing this dependency)

  • lfshook is a logrus hook that writes log entries to files based on log level
  • Reason for removal: only used to write all levels to a single file — a trivial logrus.Hook implementation. The library is not actively maintained (last release 2019)
  • go.mod impact: fully removed from go.mod — no other module in the dependency tree uses lfshook, so go mod tidy eliminates it entirely

What (replacement details)

  • Replace lfshook.NewHook(PathMap, nil) with a 20-line fileHook struct implementing logrus.Hook
  • logging/logutil.go: removed lfshook import, added fileHook type with Levels() and Fire() methods, replaced the hook registration call

Safety (why this is safe)

  • Risk level: low
  • fileHook.Levels() returns logrus.AllLevels (same as the original PathMap mapping all levels)
  • fileHook.Fire() opens the file in append mode, formats the entry with logrus.TextFormatter, and writes — same behavior as lfshook's internal logic
  • Uses DisableColors: true in the formatter to match lfshook's default behavior (no ANSI codes in file output)

Test plan

  • TestFileHookLevels - verifies Levels() returns all logrus levels
  • TestFileHookFire - verifies Fire() writes a formatted entry to the file
  • TestFileHookFireAppends - verifies multiple Fire() calls append (not overwrite)
  • TestFileHookIntegration - end-to-end: adds hook to logrus logger, logs a message, verifies file output
  • go build ./cmd/... pass
  • go test ./logging/... pass

Review hint (how to review efficiently)

  1. Read fileHook struct + Levels() + Fire() (20 lines) — this is the entire replacement
  2. The hook registration site change is minimal: lfshook.NewHook(PathMap{...}, nil) becomes &fileHook{path: path, formatter: &logrus.TextFormatter{DisableColors: true}}
  3. go.mod diff shows lfshook completely removed — verify with go mod why -m github.com/rifflock/lfshook

🤖 Generated with Claude Code

kotakanbe and others added 7 commits March 16, 2026 13:08
Replace github.com/rifflock/lfshook with a minimal fileHook that
implements logrus.Hook. The hook writes log entries to a file using
logrus.TextFormatter. lfshook is now fully removed from the module.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Verify that fileHook correctly implements logrus.Hook interface,
writes formatted entries to file, and appends on multiple calls.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@kotakanbe kotakanbe requested a review from Copilot March 24, 2026 00:59
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the unmaintained lfshook dependency and replaces it with a small in-repo logrus.Hook implementation that writes log entries to a local file.

Changes:

  • Removed github.com/rifflock/lfshook from go.mod/go.sum.
  • Added a fileHook type implementing logrus.Hook and swapped hook registration to use it.
  • Added unit/integration tests validating fileHook behavior.

Reviewed changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.

File Description
logging/logutil.go Replaces lfshook usage with a local fileHook implementation and registers it on the logger.
logging/filehook_test.go Adds tests to validate fileHook levels, write behavior, append behavior, and end-to-end integration.
go.mod Removes the lfshook requirement.
go.sum Removes lfshook checksums after go mod tidy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines 118 to 124
path := filepath.Join(dir, fmt.Sprintf("%s.log", whereami))
if _, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644); err == nil {
log.Hooks.Add(lfshook.NewHook(lfshook.PathMap{
logrus.DebugLevel: path,
logrus.InfoLevel: path,
logrus.WarnLevel: path,
logrus.ErrorLevel: path,
logrus.FatalLevel: path,
logrus.PanicLevel: path,
}, nil))
log.Hooks.Add(&fileHook{
path: path,
formatter: &logrus.TextFormatter{DisableColors: true},
})
} else {
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os.OpenFile call opens a file descriptor that is never closed (the returned *os.File is discarded). This can leak a FD per logger initialization. Use os.OpenFile to get f, close it (or use os.OpenFile inside a small scope and defer f.Close()), or replace this check with an operation that doesn't allocate an FD (e.g., attempt to open with proper close, or rely on the hook's Fire() to surface errors).

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The os.OpenFile at line 119 (in NewCustomLogger) was only used to verify the file could be created, but the returned *os.File was discarded without closing. Fixed: renamed to checkFile and added checkFile.Close() immediately after the check succeeds.

Comment on lines +26 to +38
func (h *fileHook) Fire(entry *logrus.Entry) error {
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
if err != nil {
return err
}
defer f.Close()

b, err := h.formatter.Format(entry)
if err != nil {
return err
}
_, err = f.Write(b)
return err
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f.Write(b) can legally perform a short write without returning an error, which would truncate log entries. Consider looping until all bytes are written (or use an io.Copy-style approach) and also consider capturing/returning Close() errors (currently ignored via defer f.Close()).

Suggested change
func (h *fileHook) Fire(entry *logrus.Entry) error {
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
if err != nil {
return err
}
defer f.Close()
b, err := h.formatter.Format(entry)
if err != nil {
return err
}
_, err = f.Write(b)
return err
func (h *fileHook) Fire(entry *logrus.Entry) (retErr error) {
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
if err != nil {
return err
}
defer func() {
if cerr := f.Close(); cerr != nil && retErr == nil {
retErr = cerr
}
}()
b, err := h.formatter.Format(entry)
if err != nil {
return err
}
total := 0
for total < len(b) {
n, werr := f.Write(b[total:])
if werr != nil {
return werr
}
if n == 0 {
return io.ErrShortWrite
}
total += n
}
return nil

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fixing. Per the io.Writer contract, a short write must return a non-nil error (Write must return a non-nil error if it returns n < len(p)). The *os.File.Write implementation follows this contract. Adding a write loop would be dead code since any short write already surfaces as an error. Similarly, defer f.Close() already handles close — capturing the close error in a named return would add complexity with no practical benefit for a log hook.

}

func (h *fileHook) Levels() []logrus.Level {
return logrus.AllLevels
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning logrus.AllLevels directly exposes a shared slice; a caller could mutate it and affect global behavior. Prefer returning a copy (e.g., copy into a new slice) to prevent accidental mutation.

Suggested change
return logrus.AllLevels
levels := make([]logrus.Level, len(logrus.AllLevels))
copy(levels, logrus.AllLevels)
return levels

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not fixing. fileHook is an unexported type, so Levels() is only called by the logrus hook dispatch internally. No external caller can mutate the returned slice. Defensive copying would add an allocation on every log entry for a scenario that cannot occur in practice.


func TestFileHookLevels(t *testing.T) {
h := &fileHook{
path: "/dev/null",
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using \"/dev/null\" will fail on non-Unix platforms. Use os.DevNull for a portable sink path.

Suggested change
path: "/dev/null",
path: os.DevNull,

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Changed to os.DevNull for cross-platform portability.

}

func (h *fileHook) Fire(entry *logrus.Entry) error {
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating log files with mode 0644 makes them readable by other users on the host, which can expose sensitive data if logs contain secrets/tokens. Consider using 0600 by default (or making the mode configurable) to reduce unintended disclosure.

Suggested change
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0644)
f, err := os.OpenFile(h.path, os.O_CREATE|os.O_WRONLY|os.O_APPEND, 0600)

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Changed all log file OpenFile calls from 0644 to 0600 — both in the fileHook.Fire method and in NewCustomLogger (for vuls.log and the per-host log file).

- Close FD leak in NewCustomLogger (checkFile.Close())
- Use os.DevNull instead of /dev/null in test for portability
- Tighten file permissions from 0644 to 0600 for all log files

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated no new comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants