Migrate logging framework from Zap to Zerolog#368
Merged
nicholas-fedor merged 23 commits intomainfrom Mar 2, 2026
Merged
Conversation
- Update dependencies to charm.land/bubbletea/v2 and lipgloss/v2 - Change View() signature from string to tea.View return type - Replace tea.WithAltScreen() option with view.AltScreen field - Remove tea.EnterAltScreen command from Init() method - Update tests to use new tea.KeyPressMsg interface - Add Windows build exclusions for riscv64 and arm architectures
- Configure setup-go action to read Go version from go.mod file - Remove hardcoded GO_VERSION environment variables - Remove Go version matrix from test workflow
- Simplify project description in README - Update mockery configuration to use latest version - Add ./bin directory to gitignore - Remove redundant windows/riscv64 ignore from goreleaser
- Add gocritic and revive configurations to .golangci.yaml - Rename parameters to avoid package name conflicts - Refactor test assertions to use literal path strings - Apply consistent formatting with blank lines and append patterns
- Replace hardcoded Unix-style paths with filepath.Join() calls - Update test cases for DetermineBinDir and AdjustBinaryPath - Ensure Windows compatibility for path separators
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Nick Fedor <nick@nickfedor.com>
- Replace filepath.Join with filepath.FromSlash for OS-agnostic paths - Ensure correct path separators on Windows and Unix systems
…icholas-fedor/go-remove into fix/bubbletea-v2-api-migration
- Replace zap logger with zerolog for improved performance and simpler API - Update logger interface to use level-specific methods instead of SugaredLogger - Add SetCaptureFunc and Level methods to support test log capture and runtime level changes - Simplify root command initialization by removing redundant logger setup - Update all mocks and tests to align with the new zerolog-based interface
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughReplaces Zap with Zerolog across the codebase, expands the Logger interface (per-level methods, Sync, Level, capture support), integrates non‑blocking log capture into the TUI (log channel, circular buffer, log panel and "L" toggle), updates logger mocks and tests, adjusts filesystem logging calls, and migrates dependencies (Bubble Tea/Lipgloss v2, add zerolog, remove zap). Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TUI as TUI Model
participant Logger as Logger
participant Capture as CaptureWriter
participant LogChan as Log Channel
participant UI as UI Renderer
User->>TUI: Start TUI (verbose)
TUI->>Logger: NewLoggerWithCapture()
Logger->>Capture: create capture writer
Capture->>LogChan: wire capture bridge -> non-blocking send
rect rgba(100,150,200,0.5)
Note over Logger,Capture: Application emits logs
Logger->>Capture: Write(logLine)
Capture->>LogChan: Send LogMsg (non-blocking, drop if full)
end
rect rgba(150,100,200,0.5)
Note over TUI,UI: UI polls and renders logs
TUI->>LogChan: pollLogChannel()
LogChan->>TUI: LogMsg
TUI->>TUI: addLogEntry (circular buffer)
TUI->>UI: View() render log panel
UI->>User: Display logs
end
User->>TUI: Press "L"
TUI->>TUI: Toggle showLogs
TUI->>UI: Update View
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Signed-off-by: Nick Fedor <nick@nickfedor.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cli/cli_test.go (1)
196-212:⚠️ Potential issue | 🟡 MinorType assertion will fail for
mockLoggerwhenVerbose=true.The local
runfunction performs a type assertion to*logger.ZerologLogger(line 201), but tests usemockLogger. If any test case setsVerbose=true, the assertion fails and returnsErrInvalidLoggerType. Current tests all useVerbose=false, so this works, but it's fragile.Consider either:
- Removing the type assertion (call
Level()directly on the interface since it's part oflogger.Logger)- Adding a comment warning future test authors
🐛 Proposed fix: use interface method directly
if config.Verbose { - zl, ok := log.(*logger.ZerologLogger) - if !ok { - return fmt.Errorf( - "failed to set log level: %w with type %T", - ErrInvalidLoggerType, - log, - ) - } - - level := logger.ParseLevel(config.LogLevel) - zl.Level(level) + level := logger.ParseLevel(config.LogLevel) + log.Level(level) // Level() is part of logger.Logger interface }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 196 - 212, The run function in the test uses a concrete type assertion to *logger.ZerologLogger when config.Verbose is true, which will fail for test doubles like mockLogger; instead call the Level method via the logger interface (use deps.Logger.Level(logger.ParseLevel(config.LogLevel))) or ensure the logger interface in Dependencies exposes Level so mockLogger can implement it, and remove the ErrInvalidLoggerType type assertion involving logger.ZerologLogger to avoid breaking tests that set Config.Verbose.
🧹 Nitpick comments (6)
.golangci.yaml (2)
128-128: Consider enablingzerologlintsince this PR migrates to zerolog.The PR description states the project is migrating from zap to zerolog. The
zerologlintlinter detects cases where zerolog chains are missing the finalSend()orMsg()call, which silently drops log messages. This is a common pitfall with zerolog's fluent API.♻️ Suggested change
Move
zerologlintfrom thedisablelist toenable, or simply remove it fromdisableif usingdefault: all:- - zerologlint # Detects the wrong usage of `zerolog` that a user forgets to dispatch with `Send` or `Msg`.And add to the enable list:
+ - zerologlint # Detects the wrong usage of `zerolog` that a user forgets to dispatch with `Send` or `Msg`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml at line 128, The linter setting currently keeps zerologlint disabled even though the codebase migrated to zerolog; update the .golangci.yaml to enable zerologlint by removing "zerologlint" from the disable list and adding it to the enable list (or simply delete it so default: all picks it up), ensuring zerologlint is active to catch missing Msg()/Send() calls in zerolog usage.
143-153: The nested list structure is correct—no fix needed for the- -syntax.The official golangci-lint documentation confirms that the third argument (options map) uses exactly the
- -structure shown in your configuration. This is proper YAML syntax for representing a map within an arguments list, not an incorrect nested structure.However, consider simplifying the empty list initialisms:
- Change
[""](list with one empty string) to[](empty list) for AllowList and DenyList. An empty list is cleaner and more semantically correct than a list containing an empty string.- The
exclude: [""]should also use[]for consistency.No action required for the nested list structure; only the empty string arrays warrant review.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yaml around lines 143 - 153, The YAML uses lists containing an empty string which is misleading; update the keys `exclude` and the two argument entries currently written as `[""]` to be empty lists `[]` instead (i.e., replace the `[""]` values for `exclude`, the AllowList and DenyList argument entries with `[]`) while leaving the existing nested options map (the `- -` block with keys like `skip-initialism-name-checks`, `upper-case-const`, `extra-bad-package-names`) unchanged..mockery.yaml (1)
15-15: Consider pinning the Mockery version for reproducibility.Using
@latestcan lead to inconsistent mock generation across different environments or CI runs when Mockery releases breaking changes. Pinning to a specific version (e.g., the previousv3.5.2or newer) ensures reproducible builds.-# go install github.com/vektra/mockery/v3@latest # +# go install github.com/vektra/mockery/v3@v3.5.2 #🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.mockery.yaml at line 15, Replace the use of the floating "go install github.com/vektra/mockery/v3@latest" reference in the .mockery.yaml comment with a pinned release tag; specifically update the install line (the string "go install github.com/vektra/mockery/v3@latest") to use a fixed version like "@v3.5.2" (or another chosen semver) so mock generation is reproducible across environments and CI.cmd/root.go (2)
87-91: DiscardedcaptureWriterreturn value.The
captureWriteris discarded. Based oninternal/logger/logger.go:177-215, the capture mechanism is wired internally to the logger viaSetCaptureFunc, so discarding the writer appears intentional. Consider adding a brief comment to clarify this is expected.📝 Optional: Add clarifying comment
// Initialize the logger with capture support for TUI mode. -log, _, err := logger.NewLoggerWithCapture() +// The captureWriter is internal to the logger; capture is configured via SetCaptureFunc later. +log, _, err := logger.NewLoggerWithCapture()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 87 - 91, The returned captureWriter from NewLoggerWithCapture is intentionally ignored because the capture mechanism is hooked into the logger via SetCaptureFunc in internal/logger/logger.go; add a brief inline comment next to the call in cmd/root.go (where NewLoggerWithCapture is invoked and captureWriter is currently discarded) explaining that captureWriter is intentionally not used because SetCaptureFunc wires capture internally, to avoid future confusion.
33-34: Duplicate error variable: consolidateErrInvalidLoggerType.This error is defined identically in both
cmd/root.goandinternal/cli/cli.go(line 31). Consider consolidating to a single definition in one package and importing where needed to avoid drift.♻️ Suggested consolidation
Keep the definition in
internal/cli/cli.go(orinternal/logger/logger.go) and remove fromcmd/root.go:-// ErrInvalidLoggerType indicates that the logger is not of the expected *ZerologLogger type. -var ErrInvalidLoggerType = errors.New("logger is not a *ZerologLogger")Then import/use from the canonical location where needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 33 - 34, There are two identical ErrInvalidLoggerType error variables; remove the duplicate definition from the cmd/root.go location and use the single canonical ErrInvalidLoggerType defined in internal/cli/cli.go (or move it to a shared internal/logger package if preferred). Update any references to ErrInvalidLoggerType in cmd/root.go to import and reference the canonical variable from the package that contains the retained definition (ensure the import alias matches any existing package names and that tests/consumers compile).internal/fs/fs_test.go (1)
263-272: Consider simplifying mock setup or documenting the pattern.The mock setup creates a real zerolog logger with
nilwriter just to obtain a*zerolog.Eventto return from mock methods. While this works, it's a bit convoluted. The pattern is reused inTestRealFS_RemoveBinary_VerboseLogging(lines 309-315), so this is consistent.💡 Optional: Extract a helper for creating dummy events
// dummyEvent returns a zerolog.Event suitable for mock return values. func dummyEvent() *zerolog.Event { return zerolog.New(nil).Debug() }Then use it in test cases:
logger: func() logger.Logger { log := mocks.NewMockLogger(t) - zl := zerolog.New(nil).With().Logger() - dummyEvent := zl.Debug() + dummyEvent := dummyEvent() log.On("Debug").Return(dummyEvent) log.On("Info").Return(dummyEvent) return log },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fs/fs_test.go` around lines 263 - 272, The mock setup in the logger factory is creating a real zerolog Logger just to obtain a *zerolog.Event (dummyEvent := zl.Debug()), which is repeated in TestRealFS_RemoveBinary_VerboseLogging; extract a small helper function (e.g., dummyEvent() returning *zerolog.Event) and replace the inlined creation in the anonymous logger func and other tests, and update mocks.NewMockLogger(t).On("Debug").Return(...) and .On("Info").Return(...) to use that helper to simplify and document the pattern.
🤖 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/cli/tui.go`:
- Around line 264-267: When toggling the log panel (flip of m.showLogs inside
the "L" case) and after appending new log entries, call the UI layout
recalculation function updateGrid so the reserved height/row/column sizing is
recomputed; update the "L" case to call updateGrid() immediately after
m.showLogs = !m.showLogs, and ensure any function that appends logs (the code
around where logs are pushed/updated near line ~312) also calls updateGrid()
after modifying the logs so the grid sizing is refreshed.
- Around line 195-197: The log polling currently only runs on demand and can
stall; add a time import and introduce a new tick message type (logPollTickMsg)
and refactor pollLogChannel() to use tea.Tick(100*time.Millisecond, ...) that
sends logPollTickMsg and self-reschedules; update the model's Update() to handle
logPollTickMsg by returning the next poll command, remove the unconditional
footer polling logic, and simplify the LogMsg case to return the rescheduled
poll command directly so logs are continuously polled every 100ms even when the
UI is idle.
In `@internal/logger/logger_test.go`:
- Around line 328-387: TestZerologLogger_ConcurrentAccess uses a shared
bytes.Buffer which is not safe for concurrent writes; create a small
synchronized writer type (e.g., a struct with a bytes.Buffer and sync.Mutex that
implements io.Writer and a locked String() accessor) and use an instance of that
type instead of the raw bytes.Buffer for zerolog.ConsoleWriter.Out, update
references from buf.String() to the synchronized writer's String(), and keep
using ZerologLogger, output and TestZerologLogger_ConcurrentAccess as the
locations to change.
In `@internal/logger/logger.go`:
- Around line 85-95: The bridge callback (captureWriter.captureFunc) is being
initialized to a non-nil sentinel so the writer always takes the discard branch
and drops logs; change the logic so we only treat capture as enabled when a real
user callback is set. Locate captureWriter.captureFunc and the write branch that
calls captureWriter.captureLogMessage(...) and io.Discard.Write(...), and modify
initialization or the conditional check: either initialize
captureWriter.captureFunc to nil when no UI callback exists or add a sentinel
check (e.g., compare to the bridge function symbol) before invoking
captureLogMessage and writing to io.Discard. Ensure the write path only runs
when a real capture callback is present and falls back to the normal writer
behavior when capture is disabled.
---
Outside diff comments:
In `@internal/cli/cli_test.go`:
- Around line 196-212: The run function in the test uses a concrete type
assertion to *logger.ZerologLogger when config.Verbose is true, which will fail
for test doubles like mockLogger; instead call the Level method via the logger
interface (use deps.Logger.Level(logger.ParseLevel(config.LogLevel))) or ensure
the logger interface in Dependencies exposes Level so mockLogger can implement
it, and remove the ErrInvalidLoggerType type assertion involving
logger.ZerologLogger to avoid breaking tests that set Config.Verbose.
---
Nitpick comments:
In @.golangci.yaml:
- Line 128: The linter setting currently keeps zerologlint disabled even though
the codebase migrated to zerolog; update the .golangci.yaml to enable
zerologlint by removing "zerologlint" from the disable list and adding it to the
enable list (or simply delete it so default: all picks it up), ensuring
zerologlint is active to catch missing Msg()/Send() calls in zerolog usage.
- Around line 143-153: The YAML uses lists containing an empty string which is
misleading; update the keys `exclude` and the two argument entries currently
written as `[""]` to be empty lists `[]` instead (i.e., replace the `[""]`
values for `exclude`, the AllowList and DenyList argument entries with `[]`)
while leaving the existing nested options map (the `- -` block with keys like
`skip-initialism-name-checks`, `upper-case-const`, `extra-bad-package-names`)
unchanged.
In @.mockery.yaml:
- Line 15: Replace the use of the floating "go install
github.com/vektra/mockery/v3@latest" reference in the .mockery.yaml comment with
a pinned release tag; specifically update the install line (the string "go
install github.com/vektra/mockery/v3@latest") to use a fixed version like
"@v3.5.2" (or another chosen semver) so mock generation is reproducible across
environments and CI.
In `@cmd/root.go`:
- Around line 87-91: The returned captureWriter from NewLoggerWithCapture is
intentionally ignored because the capture mechanism is hooked into the logger
via SetCaptureFunc in internal/logger/logger.go; add a brief inline comment next
to the call in cmd/root.go (where NewLoggerWithCapture is invoked and
captureWriter is currently discarded) explaining that captureWriter is
intentionally not used because SetCaptureFunc wires capture internally, to avoid
future confusion.
- Around line 33-34: There are two identical ErrInvalidLoggerType error
variables; remove the duplicate definition from the cmd/root.go location and use
the single canonical ErrInvalidLoggerType defined in internal/cli/cli.go (or
move it to a shared internal/logger package if preferred). Update any references
to ErrInvalidLoggerType in cmd/root.go to import and reference the canonical
variable from the package that contains the retained definition (ensure the
import alias matches any existing package names and that tests/consumers
compile).
In `@internal/fs/fs_test.go`:
- Around line 263-272: The mock setup in the logger factory is creating a real
zerolog Logger just to obtain a *zerolog.Event (dummyEvent := zl.Debug()), which
is repeated in TestRealFS_RemoveBinary_VerboseLogging; extract a small helper
function (e.g., dummyEvent() returning *zerolog.Event) and replace the inlined
creation in the anonymous logger func and other tests, and update
mocks.NewMockLogger(t).On("Debug").Return(...) and .On("Info").Return(...) to
use that helper to simplify and document the pattern.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (22)
.github/workflows/build.yaml.github/workflows/lint.yaml.github/workflows/security.yaml.github/workflows/test.yaml.gitignore.golangci.yaml.goreleaser.yaml.mockery.yamlREADME.mdcmd/root.gocmd/root_test.gogo.modinternal/cli/cli.gointernal/cli/cli_test.gointernal/cli/mocks/ProgramRunner.gointernal/cli/tui.gointernal/cli/tui_test.gointernal/fs/fs.gointernal/fs/fs_test.gointernal/logger/logger.gointernal/logger/logger_test.gointernal/logger/mocks/Logger.go
💤 Files with no reviewable changes (1)
- cmd/root_test.go
Comment on lines
+195
to
+197
| if m.config.Verbose && m.logChan != nil { | ||
| return m.pollLogChannel() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "tui.go" -path "*/cli/*"Repository: nicholas-fedor/go-remove
Length of output: 89
🏁 Script executed:
head -n 1 internal/cli/tui.go | wc -c
wc -l internal/cli/tui.goRepository: nicholas-fedor/go-remove
Length of output: 93
🏁 Script executed:
sed -n '1,30p' internal/cli/tui.go
sed -n '195,220p' internal/cli/tui.go
sed -n '310,330p' internal/cli/tui.goRepository: nicholas-fedor/go-remove
Length of output: 2128
🏁 Script executed:
sed -n '190,350p' internal/cli/tui.go | cat -nRepository: nicholas-fedor/go-remove
Length of output: 5387
🌐 Web query:
Bubble Tea v2 Cmd nil behavior no-op Go
💡 Result:
In Bubble Tea v2, tea.Cmd is defined as:
type Cmd func() Msg
and a nil Cmd is explicitly treated as a no-op (i.e., “no command / no I/O right now”). So it’s correct and idiomatic to return nil for the command from Init/Update when you don’t want to run any async work. [1]
Sources
[1] pkg.go.dev — github.com/charmbracelet/bubbletea/v2 docs for type Cmd (pkg.go.dev)
Citations:
🌐 Web query:
Bubble Tea v2 Tick function one-shot documentation
💡 Result:
In Bubble Tea v2, tea.Tick is a one-shot timer command:
func Tick(d time.Duration, fn func(time.Time) Msg) CmdIt starts a timer when invoked and, after d, sends exactly one message produced by fn. It does not repeat automatically—if you want an interval, return another tea.Tick(...) again after you receive your tick message in Update. [1]
A repeating pattern from the docs:
type TickMsg time.Time
func doTick() tea.Cmd {
return tea.Tick(time.Second, func(t time.Time) tea.Msg {
return TickMsg(t)
})
}
func (m model) Init() (tea.Model, tea.Cmd) { return m, doTick() }
func (m model) Update(msg tea.Msg) (tea.Model, tea.Cmd) {
switch msg.(type) {
case TickMsg:
return m, doTick() // re-schedule next tick
}
return m, nil
}
``` [1]
**Source:** Bubble Tea v2 package docs (`charm.land/bubbletea/v2`) [1]
---
[1] https://pkg.go.dev/charm.land/bubbletea/v2
---
</details>
**Implement continuous log polling to prevent log streaming stalls.**
The current implementation only polls the log channel on demand—during `Init()` and when other `Update()` events occur. When `pollLogChannel()` returns `nil` on an empty channel, no command is scheduled, so background logs arriving while the UI is idle won't be captured until the next user interaction (keyboard input, window resize, etc.).
With Bubble Tea v2's one-shot `Tick` behavior confirmed, the proposed fix using self-rescheduling tick messages is the correct solution. Implement the suggested fix to ensure logs are polled continuously every 100ms regardless of UI activity:
- Add `time` import and `logPollTickMsg` type
- Change `pollLogChannel()` to use `tea.Tick(100*time.Millisecond, ...)` with self-rescheduling
- Handle `logPollTickMsg` in `Update()` to reschedule the next tick
- Remove the unconditional footer polling logic in `Update()` (lines 321-327)
- Simplify the `LogMsg` case to directly return the rescheduled poll command
<details>
<summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In @internal/cli/tui.go around lines 195 - 197, The log polling currently only
runs on demand and can stall; add a time import and introduce a new tick message
type (logPollTickMsg) and refactor pollLogChannel() to use
tea.Tick(100*time.Millisecond, ...) that sends logPollTickMsg and
self-reschedules; update the model's Update() to handle logPollTickMsg by
returning the next poll command, remove the unconditional footer polling logic,
and simplify the LogMsg case to return the rescheduled poll command directly so
logs are continuously polled every 100ms even when the UI is idle.
</details>
<!-- fingerprinting:phantom:medusa:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
- Enable zerologlint linter in golangci-lint configuration - Fix mock loggers to dispatch events with Msg() calls - Remove unused ErrInvalidLoggerType and type assertion - Add nolint comments for factory method returns - Fix revive linter empty slice syntax
- Update grid layout when toggling log panel visibility with 'L' key - Update grid layout when receiving new log messages
- Add syncBuffer type with mutex-protected Write and String methods - Update TestZerologLogger_ConcurrentAccess to use syncBuffer instead of bytes.Buffer
- Add captureEnabled flag for thread-safe state tracking in Write - Add captureWriter field to ZerologLogger for lifecycle management - Refactor SetCaptureFunc to use SetupCaptureBridge for safety
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (28.05%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. @@ Coverage Diff @@
## main #368 +/- ##
===========================================
- Coverage 69.68% 56.23% -13.46%
===========================================
Files 6 6
Lines 353 489 +136
===========================================
+ Hits 246 275 +29
- Misses 97 197 +100
- Partials 10 17 +7
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/logger/logger.go (2)
77-106: Unnecessary error handling for io.Discard.Write.
io.Discard.Write()always succeeds and never returns an error (it's documented to returnlen(p), nil). The error wrapping on Lines 91-94 adds complexity without benefit.♻️ Simplify discard write
if enabled { w.captureLogMessage(string(data)) - // Write to io.Discard to satisfy the writer interface without producing output. - bytesWritten, err := io.Discard.Write(data) - if err != nil { - return bytesWritten, fmt.Errorf("failed to write to discard: %w", err) - } - - return bytesWritten, nil + return len(data), nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/logger/logger.go` around lines 77 - 106, The Write method on captureWriter currently calls io.Discard.Write and checks its error even though io.Discard.Write always returns (len(p), nil); remove the unnecessary error handling in captureWriter.Write (the method and its captureEnabled branch using captureLogMessage), and simply write to io.Discard while returning the number of bytes written (len(data)) and nil directly (or call io.Discard.Write and ignore its error), eliminating the fmt.Errorf wrapping and conditional error branch.
108-139: Log message parsing assumes specific ConsoleWriter format.The parsing logic expects the format
"<timestamp> <LEVEL> <message>"with the level at index 1. This tightly couples the parser to zerolog'sConsoleWriterdefault format. If the format changes (e.g., custom formatters or field ordering), parsing will break silently.Consider adding a comment documenting this dependency, or using a more robust parsing approach.
📝 Add format dependency comment
// captureLogMessage parses and captures the log message. -// Expected format from ConsoleWriter: "<timestamp> <LEVEL> <message>". +// Expected format from ConsoleWriter: "<timestamp> <LEVEL> <message>". +// NOTE: This parser depends on zerolog.ConsoleWriter's default output format. +// If the TimeFormat or other ConsoleWriter settings change the field order, +// this parsing logic must be updated accordingly. func (w *captureWriter) captureLogMessage(logLine string) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/logger/logger.go` around lines 108 - 139, The captureLogMessage function in captureWriter relies on ConsoleWriter's "<timestamp> <LEVEL> <message>" layout and extracts level by taking parts[1], which is fragile; update captureLogMessage (and mention captureWriter/captureFunc/capture) to either document this format dependency with a clear comment above the function and expected tokens, and/or make the parser tolerant: scan fields for a known level token set (e.g., DBG, INFO, WARN, ERROR, FATAL) instead of assuming index 1, compute msg by slicing after the matched level, and default to level "LOG" when no known token is found so capture(level, msg) still receives sensible values.internal/cli/tui.go (1)
159-160: Program reference stored after RunProgram but before Run - potential race.The program reference is stored at Line 160, but this happens after
runner.RunProgram(m)returns and beforeprogram.Run(). Sincem.programis accessed bypollLogChannelwhich is started inInit(), andInit()is called duringRun(), there's a potential timing issue.However, since
pollLogChannelonly sends to the channel and doesn't usem.programdirectly, this appears safe. The comment about storing for "log message sending" seems misleading sincem.programisn't actually used in the current implementation.💡 Consider removing unused field or clarifying intent
If
m.programis intended for future use (e.g., callingprogram.Send()), consider adding a comment explaining the planned usage. Otherwise, remove the unused field to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/tui.go` around lines 159 - 160, m.program is stored after runner.RunProgram(m) but never actually used by pollLogChannel, which is confusing and risks a race if later used; either remove the m.program field and its assignment entirely (delete the field on the model struct and the line "m.program = program") and any dead references, or if you intend to use program.Send()/other API later, move the assignment so the model gets program before Run (assign m.program = program immediately when program is created or passed in) and add a short comment on the model field explaining its intended future use and threading assumptions; locate references by the symbols m.program, runner.RunProgram, program.Run, pollLogChannel, Init, and Run.
🤖 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/cli/cli_test.go`:
- Around line 54-91: The mockLogger's Debug/Info/Warn/Error currently discard
the event and return nil which can cause nil derefs; change those methods to
return the *zerolog.Event produced by m.nopLogger.Debug()/Info()/Warn()/Error()
(so callers can chain on the returned event), add a helper constructor
newMockLogger() that initializes nopLogger appropriately, and update test usages
from &mockLogger{} to newMockLogger(); leave Sync and SetCaptureFunc behavior
as-is.
In `@internal/cli/tui_test.go`:
- Around line 37-67: The tuiMockLogger's Debug/Info/Warn/Error methods call
m.nopLogger.<Level>().Msg("") and then return nil, which mirrors the same
nil-event bug in cli_test.go; update those methods (Debug, Info, Warn, Error on
tuiMockLogger) to return the zerolog.Event produced by m.nopLogger (e.g., return
m.nopLogger.Debug() without discarding it) so tests see a real event, and
refactor by extracting a shared mock (e.g., sharedMockLogger or mockLogger)
implementing Debug/Info/Warn/Error/Sync/Level/SetCaptureFunc used by both
tui_test.go and cli_test.go to avoid duplication and ensure consistent behavior.
In `@internal/fs/fs_test.go`:
- Around line 306-333: The test finalizes the zerolog event too early and reuses
the same event for both Debug() and Info(): remove the dummyEvent.Msg("") call
and instead return two separate non-finalized events from the mock (e.g. create
dummyDebug := zl.Debug() and dummyInfo := zl.Debug()/Info() and use
log.On("Debug").Return(dummyDebug) and log.On("Info").Return(dummyInfo)); update
TestRealFS_RemoveBinary_VerboseLogging so Debug and Info expectations return
distinct, non-finalized events rather than calling Msg() during setup.
- Around line 263-273: The mock logger setup finalizes a zerolog.Event
(dummyEvent) by calling dummyEvent.Msg("") and then returns that same finalized
event for both Debug() and Info(), which will break chaining; change the mock to
return a new, unfired zerolog.Event for each call instead of reusing a finalized
one — e.g., create the zerolog.Logger via zerolog.New(nil).With().Logger() and
have mocks.NewMockLogger(t) expectations for Debug and Info return fresh events
(call logger.Debug() / logger.Info() inside the mock setup each time) so Debug()
and Info() each get an unused event rather than the finalized dummyEvent
referenced here.
---
Nitpick comments:
In `@internal/cli/tui.go`:
- Around line 159-160: m.program is stored after runner.RunProgram(m) but never
actually used by pollLogChannel, which is confusing and risks a race if later
used; either remove the m.program field and its assignment entirely (delete the
field on the model struct and the line "m.program = program") and any dead
references, or if you intend to use program.Send()/other API later, move the
assignment so the model gets program before Run (assign m.program = program
immediately when program is created or passed in) and add a short comment on the
model field explaining its intended future use and threading assumptions; locate
references by the symbols m.program, runner.RunProgram, program.Run,
pollLogChannel, Init, and Run.
In `@internal/logger/logger.go`:
- Around line 77-106: The Write method on captureWriter currently calls
io.Discard.Write and checks its error even though io.Discard.Write always
returns (len(p), nil); remove the unnecessary error handling in
captureWriter.Write (the method and its captureEnabled branch using
captureLogMessage), and simply write to io.Discard while returning the number of
bytes written (len(data)) and nil directly (or call io.Discard.Write and ignore
its error), eliminating the fmt.Errorf wrapping and conditional error branch.
- Around line 108-139: The captureLogMessage function in captureWriter relies on
ConsoleWriter's "<timestamp> <LEVEL> <message>" layout and extracts level by
taking parts[1], which is fragile; update captureLogMessage (and mention
captureWriter/captureFunc/capture) to either document this format dependency
with a clear comment above the function and expected tokens, and/or make the
parser tolerant: scan fields for a known level token set (e.g., DBG, INFO, WARN,
ERROR, FATAL) instead of assuming index 1, compute msg by slicing after the
matched level, and default to level "LOG" when no known token is found so
capture(level, msg) still receives sensible values.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.golangci.yamlinternal/cli/cli.gointernal/cli/cli_test.gointernal/cli/tui.gointernal/cli/tui_test.gointernal/fs/fs_test.gointernal/logger/logger.gointernal/logger/logger_test.go
💤 Files with no reviewable changes (1)
- internal/cli/cli.go
Comment on lines
+37
to
+67
| // tuiMockLogger is a simple mock logger for TUI tests. | ||
| type tuiMockLogger struct { | ||
| nopLogger zerolog.Logger | ||
| } | ||
|
|
||
| func (m *tuiMockLogger) Debug() *zerolog.Event { | ||
| m.nopLogger.Debug().Msg("") | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (m *tuiMockLogger) Info() *zerolog.Event { | ||
| m.nopLogger.Info().Msg("") | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (m *tuiMockLogger) Warn() *zerolog.Event { | ||
| m.nopLogger.Warn().Msg("") | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (m *tuiMockLogger) Error() *zerolog.Event { | ||
| m.nopLogger.Error().Msg("") | ||
|
|
||
| return nil | ||
| } | ||
| func (m *tuiMockLogger) Sync() error { return nil } | ||
| func (m *tuiMockLogger) Level(_ zerolog.Level) {} | ||
| func (m *tuiMockLogger) SetCaptureFunc(_ logger.LogCaptureFunc) {} |
There was a problem hiding this comment.
Same nil event issue as mockLogger in cli_test.go.
The tuiMockLogger has the identical problem - Debug/Info/Warn/Error methods return nil after discarding the actual event. This creates fragile tests that may mask nil pointer issues in production code.
Consider extracting a shared mock logger implementation between cli_test.go and tui_test.go to avoid duplication and ensure consistent behavior.
🐛 Proposed fix
// tuiMockLogger is a simple mock logger for TUI tests.
type tuiMockLogger struct {
- nopLogger zerolog.Logger
+ nopLogger zerolog.Logger // should be initialized with zerolog.New(io.Discard)
}
+func newTuiMockLogger() *tuiMockLogger {
+ return &tuiMockLogger{
+ nopLogger: zerolog.New(io.Discard),
+ }
+}
+
func (m *tuiMockLogger) Debug() *zerolog.Event {
- m.nopLogger.Debug().Msg("")
-
- return nil
+ return m.nopLogger.Debug()
}
// ... similar for Info, Warn, Error🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cli/tui_test.go` around lines 37 - 67, The tuiMockLogger's
Debug/Info/Warn/Error methods call m.nopLogger.<Level>().Msg("") and then return
nil, which mirrors the same nil-event bug in cli_test.go; update those methods
(Debug, Info, Warn, Error on tuiMockLogger) to return the zerolog.Event produced
by m.nopLogger (e.g., return m.nopLogger.Debug() without discarding it) so tests
see a real event, and refactor by extracting a shared mock (e.g.,
sharedMockLogger or mockLogger) implementing
Debug/Info/Warn/Error/Sync/Level/SetCaptureFunc used by both tui_test.go and
cli_test.go to avoid duplication and ensure consistent behavior.
- Remove unnecessary error checking when writing to io.Discard - Return len(data) directly instead of capturing return values - Add explanatory comment about io.Discard.Write behavior
- Replace fixed index parsing with token scanning for level detection - Add support for variable log formats with additional prefix fields - Default to LOG level when no known zerolog level is detected
- Remove program field storing tea.Program reference from model struct - Remove program assignment initialization in RunTUI function
- Replace custom mockLogger and cliMockRunner with generated mockery mocks - Add newMockLoggerWithDefaults helper to reduce test setup boilerplate - Refactor test table to use setup functions for dependency initialization - Update assertions to verify all mock expectations are satisfied
- Remove intermediate dummyEvent variables
- Return zerolog events directly from mock configurations
- Eliminate redundant Msg("") calls on discarded events
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/cli/cli_test.go (2)
292-297: Remove or useexpectSyncCallto avoid dead test metadata.
expectSyncCallis declared in the test case struct (Line 296) but never read. Either assert on it explicitly or drop it to keep the test intent tight.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 292 - 297, The test table defines an unused boolean field expectSyncCall in the tests variable; either remove the expectSyncCall field from the test case struct or use it in the t.Run loop to assert the expected behavior. If you intend to verify whether the mock filesystem's Sync (or equivalent) was invoked, read tc.expectSyncCall after calling tc.setupFS and the code under test and assert against mockFS.MockFS (e.g., check the Sync call count or use the mock's expectation methods); otherwise simply delete expectSyncCall from the struct to avoid dead metadata.
70-286: SplitTestRuninto smaller helpers/subtests.
TestRunis doing setup wiring, execution, stdout interception, and assertions in one block. This is already over the configured size threshold (Line 74 static hint) and is getting hard to evolve safely. Please extract reusable helpers for stdout capture +run(...)execution + common assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 70 - 286, The TestRun function is too large; extract reusable helpers to reduce complexity: create a helper CaptureStdout that redirects os.Stdout and returns captured string, a helper makeDeps that sets up Dependencies from setupFS/setupLog/setupRunner (referencing Dependencies, mockFS.NewMockFS, mockLogger.NewMockLogger, DefaultRunner), and a helper executeRun that wraps the local run(...) logic (referencing the local run closure, RunTUI, deps.FS.DetermineBinDir, deps.FS.RemoveBinary) to return (output, error). Replace the inline stdout piping, dependency setup, run invocation, and repeated assertions with calls to these helpers inside smaller t.Run subtests so each test case only provides setup funcs, calls makeDeps, calls executeRun/CaptureStdout, and asserts err/output and AssertExpectations on mockFSInstance, mockLog, and mockRunner.
🤖 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/fs/fs_test.go`:
- Around line 215-216: The logger factory currently captures the parent test and
should be bound to each subtest: change the logger field type from func()
logger.Logger to func(t *testing.T) logger.Logger in the test table/struct,
update all call sites in the subtest to call tt.args.logger(t) (instead of
tt.args.logger()), and adjust any helper/fixture definitions that construct
loggers to accept the current *testing.T; apply the same change for all
occurrences around the logger declarations/usages (including the factories
referenced in the blocks around the previous lines 231-233, 251-253, 263-271 and
the usage at 295-296) so each subtest gets its own scoped logger tied to the
subtest handle.
In `@internal/logger/logger.go`:
- Around line 313-331: The SetCaptureFunc method currently skips updating the
captureWriter when captureFunc is nil, so calling SetCaptureFunc(nil) leaves the
writer in capture mode; change the conditional so that whenever z.captureWriter
!= nil you always install the bridge (using the same closure that reads
z.captureFunc under z.mu.RLock/z.mu.RUnlock) and call
z.captureWriter.SetCaptureFunc(captureFunc) (passing nil to disable) — i.e.,
remove the captureFunc != nil guard in SetCaptureFunc so the captureWriter (and
its captureEnabled state) is updated even when clearing capture.
---
Nitpick comments:
In `@internal/cli/cli_test.go`:
- Around line 292-297: The test table defines an unused boolean field
expectSyncCall in the tests variable; either remove the expectSyncCall field
from the test case struct or use it in the t.Run loop to assert the expected
behavior. If you intend to verify whether the mock filesystem's Sync (or
equivalent) was invoked, read tc.expectSyncCall after calling tc.setupFS and the
code under test and assert against mockFS.MockFS (e.g., check the Sync call
count or use the mock's expectation methods); otherwise simply delete
expectSyncCall from the struct to avoid dead metadata.
- Around line 70-286: The TestRun function is too large; extract reusable
helpers to reduce complexity: create a helper CaptureStdout that redirects
os.Stdout and returns captured string, a helper makeDeps that sets up
Dependencies from setupFS/setupLog/setupRunner (referencing Dependencies,
mockFS.NewMockFS, mockLogger.NewMockLogger, DefaultRunner), and a helper
executeRun that wraps the local run(...) logic (referencing the local run
closure, RunTUI, deps.FS.DetermineBinDir, deps.FS.RemoveBinary) to return
(output, error). Replace the inline stdout piping, dependency setup, run
invocation, and repeated assertions with calls to these helpers inside smaller
t.Run subtests so each test case only provides setup funcs, calls makeDeps,
calls executeRun/CaptureStdout, and asserts err/output and AssertExpectations on
mockFSInstance, mockLog, and mockRunner.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.golangci.yamlinternal/cli/cli_test.gointernal/cli/tui.gointernal/fs/fs_test.gointernal/logger/logger.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .golangci.yaml
- internal/cli/tui.go
- Extract captureStdout, makeDeps, and executeRun test helpers - Update FS test logger factory to accept *testing.T parameter - Fix logger SetCaptureFunc to properly disable capture with nil
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/cli/cli_test.go (1)
73-84:⚠️ Potential issue | 🟡 Minor
captureStdoutignores I/O errors and leaks the read pipe.
os.Pipe(),w.Close(), andReadFrom()errors are currently ignored, andris never closed. This can hide test failures and leak file descriptors.Run this to verify the ignored-error/read-end lifecycle in this helper:
#!/bin/bash rg -n -C2 'os\.Pipe\(|w\.Close\(|ReadFrom\(|return func\(\) string' internal/cli/cli_test.go💡 Proposed fix
func captureStdout(t *testing.T) func() string { t.Helper() oldStdout := os.Stdout - r, w, _ := os.Pipe() + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe() failed: %v", err) + } os.Stdout = w return func() string { os.Stdout = oldStdout - w.Close() + if err := w.Close(); err != nil { + t.Fatalf("failed to close stdout writer: %v", err) + } var buf bytes.Buffer - buf.ReadFrom(r) + if _, err := buf.ReadFrom(r); err != nil { + t.Fatalf("failed to read captured stdout: %v", err) + } + if err := r.Close(); err != nil { + t.Fatalf("failed to close stdout reader: %v", err) + } return buf.String() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 73 - 84, The captureStdout helper ignores errors from os.Pipe(), w.Close(), and buf.ReadFrom() and never closes the read end (r), leaking FDs and masking failures; update the captureStdout function to check the error returned by os.Pipe(), propagate or fail the test on that error, always restore os.Stdout in a defer, close the write end and check/handle the error from w.Close(), read from r into a buffer while checking and returning any ReadFrom error, and ensure r.Close() is called after reading (or deferred) so both r and w are properly closed; reference the captureStdout function and the r/w variables so the changes are made in that helper.internal/fs/fs_test.go (1)
314-315:⚠️ Potential issue | 🟡 MinorHandle
os.WriteFileerrors and tighten test file permissions.This still ignores the file creation error and uses a permissive mode, which matches earlier scanner feedback.
🔧 Proposed fix
- os.WriteFile(tmpFile, []byte("test"), 0o755) + if err := os.WriteFile(tmpFile, []byte("test"), 0o600); err != nil { + t.Fatalf("failed to create temp file %q: %v", tmpFile, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/fs/fs_test.go` around lines 314 - 315, The call to os.WriteFile creating the test file ignores the returned error and uses overly-permissive mode; update the test so the os.WriteFile(tmpFile, []byte("test"), 0o755) result is checked and handled (e.g., fail the test on error using t.Fatalf or require.NoError) and tighten the file mode to a stricter permission such as 0o600 (or 0600) to avoid world-readable/executable test artifacts; locate the call to os.WriteFile and adjust accordingly.internal/logger/logger.go (1)
321-331:⚠️ Potential issue | 🟠 MajorAvoid transient log drops when disabling capture.
The bridge is installed before capture is disabled, so concurrent writes can be discarded without reaching any callback during
SetCaptureFunc(nil). Update the writer in one step to avoid this intermediate state.🔧 Proposed fix
func (z *ZerologLogger) SetCaptureFunc(captureFunc LogCaptureFunc) { z.mu.Lock() z.captureFunc = captureFunc z.mu.Unlock() - // Set up the bridge function on the capture writer when it exists. - // The bridge function reads the current captureFunc from the logger and calls it. - // This ensures capture is properly disabled when captureFunc is nil. if z.captureWriter != nil { - z.captureWriter.SetupCaptureBridge(func(level, msg string) { - z.mu.RLock() - capture := z.captureFunc - z.mu.RUnlock() - - if capture != nil { - capture(level, msg) - } - }) z.captureWriter.SetCaptureFunc(captureFunc) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/logger/logger.go` around lines 321 - 331, Currently the bridge is installed before the writer's capture func is updated, creating a window where SetCaptureFunc(nil) can drop logs; to fix, update the writer's capture handler in one atomic step by calling z.captureWriter.SetCaptureFunc(captureFunc) first and then install the bridge (z.captureWriter.SetupCaptureBridge) with a callback that directly delegates to the writer's capture handler (so you no longer call SetCaptureFunc after installing the bridge), or refactor SetupCaptureBridge to accept the final captureFunc so both are set together; modify the code paths using z.captureWriter, SetupCaptureBridge, SetCaptureFunc, captureFunc and capture to ensure no intermediate SetCaptureFunc(nil) occurs.
🤖 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/cli/cli_test.go`:
- Around line 364-405: Tests TestRun_WithLoggerSync and TestRun_VerboseMode are
calling mock methods directly instead of exercising the Run function, so update
both tests to invoke Run(deps, config) (the Run function used by the CLI) with
the constructed Dependencies and Config objects rather than calling
deps.FS.DetermineBinDir/AdjustBinaryPath/RemoveBinary directly; keep the same
mock setup on the mockFS and mockLogger, capture stdout using captureStdout(t)
as before, call Run(deps, config), check the returned error, then assert mock
expectations (e.g., mockLog.AssertCalled/AssertExpectations and
mockFS.AssertExpectations) and any stdout side-effects to validate the real run
path.
In `@internal/logger/logger.go`:
- Around line 161-166: The code uses strings.Index(logLine, level) to locate the
message start which can match an earlier token; instead build the message from
the parsed parts slice: use parts[levelIndex+1:] (where levelIndex and parts are
from the parsed logLine) join them with spaces and assign the trimmed result to
msg instead of using msgStart/logLine; update the block that currently computes
msgStart and msg to produce msg =
strings.TrimSpace(strings.Join(parts[levelIndex+1:], " ")).
---
Duplicate comments:
In `@internal/cli/cli_test.go`:
- Around line 73-84: The captureStdout helper ignores errors from os.Pipe(),
w.Close(), and buf.ReadFrom() and never closes the read end (r), leaking FDs and
masking failures; update the captureStdout function to check the error returned
by os.Pipe(), propagate or fail the test on that error, always restore os.Stdout
in a defer, close the write end and check/handle the error from w.Close(), read
from r into a buffer while checking and returning any ReadFrom error, and ensure
r.Close() is called after reading (or deferred) so both r and w are properly
closed; reference the captureStdout function and the r/w variables so the
changes are made in that helper.
In `@internal/fs/fs_test.go`:
- Around line 314-315: The call to os.WriteFile creating the test file ignores
the returned error and uses overly-permissive mode; update the test so the
os.WriteFile(tmpFile, []byte("test"), 0o755) result is checked and handled
(e.g., fail the test on error using t.Fatalf or require.NoError) and tighten the
file mode to a stricter permission such as 0o600 (or 0600) to avoid
world-readable/executable test artifacts; locate the call to os.WriteFile and
adjust accordingly.
In `@internal/logger/logger.go`:
- Around line 321-331: Currently the bridge is installed before the writer's
capture func is updated, creating a window where SetCaptureFunc(nil) can drop
logs; to fix, update the writer's capture handler in one atomic step by calling
z.captureWriter.SetCaptureFunc(captureFunc) first and then install the bridge
(z.captureWriter.SetupCaptureBridge) with a callback that directly delegates to
the writer's capture handler (so you no longer call SetCaptureFunc after
installing the bridge), or refactor SetupCaptureBridge to accept the final
captureFunc so both are set together; modify the code paths using
z.captureWriter, SetupCaptureBridge, SetCaptureFunc, captureFunc and capture to
ensure no intermediate SetCaptureFunc(nil) occurs.
- Fix race condition in SetCaptureFunc by reordering handler updates - Correct log message extraction to use parsed parts instead of string indexing - Improve error handling in cli test helpers and fs test file operations - Update test mocks with proper zerolog initialization
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/cli_test.go (2)
135-173:executeRundiverges fromRun()— tests may not reflect actual behavior.This helper contains logic not present in the actual
Run()function (frominternal/cli/cli.go):if config.Verbose { level := logger.ParseLevel(config.LogLevel) log.Level(level) }The real
Run()doesn't set log levels — that's handled elsewhere (likely incmd/root.go). This divergence meansTestRunexercises code paths that differ from production, potentially masking bugs or giving false confidence.Consider either:
- Making
Run()accept an optional runner parameter to enable direct testing- Documenting clearly that
executeRuntests integration behavior beyondRun()- Removing the log level logic from
executeRunif it's not the responsibility of this layer🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 135 - 173, The test helper executeRun contains logger level-setting logic (the if config.Verbose { level := logger.ParseLevel(config.LogLevel); log.Level(level) } block) that diverges from the real Run() behavior; remove that block from executeRun so executeRun mirrors Run()'s responsibilities (leave RunTUI, deps.FS.DetermineBinDir, RemoveBinary, log.Sync usage unchanged) or alternatively modify Run to accept an optional ProgramRunner and move test-only behavior into the test caller; reference executeRun, Run, logger.ParseLevel, log.Level, RunTUI and ProgramRunner when making the change.
178-336: Comprehensive test coverage with minor lint warning.The test function exceeds the 50-line method limit by 3 lines (static analysis hint). This is acceptable for table-driven tests, but if you want to address it, consider extracting the test execution and assertion logic (lines 299-334) into a helper.
Note: These tests use
executeRunrather thanRun()directly, so the coverage is for the broader integration behavior including log level setting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 178 - 336, The TestRun function is 3 lines over the lint limit—extract the repeated test execution and assertions into a helper to shorten TestRun: create a helper (e.g., runTestCase or assertRunResult) that accepts the deps produced by makeDeps, the test config (Config), the runner, and the expected values, and moves the logic currently using captureStdout, executeRun, comparing err vs tt.wantErr, comparing gotOutput vs tt.wantOutput, and calling AssertExpectations for mockFSInstance, mockLog, and runner into that helper; keep TestRun as a table-driven loop that only sets up deps via makeDeps(makeDepsConfig{...}) and calls the new helper with executeRun, captureStdout, and the tt expectations to bring TestRun back under the 50-line limit while preserving use of executeRun and existing helpers.
🤖 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/cli/cli_test.go`:
- Around line 135-173: The test helper executeRun contains logger level-setting
logic (the if config.Verbose { level := logger.ParseLevel(config.LogLevel);
log.Level(level) } block) that diverges from the real Run() behavior; remove
that block from executeRun so executeRun mirrors Run()'s responsibilities (leave
RunTUI, deps.FS.DetermineBinDir, RemoveBinary, log.Sync usage unchanged) or
alternatively modify Run to accept an optional ProgramRunner and move test-only
behavior into the test caller; reference executeRun, Run, logger.ParseLevel,
log.Level, RunTUI and ProgramRunner when making the change.
- Around line 178-336: The TestRun function is 3 lines over the lint
limit—extract the repeated test execution and assertions into a helper to
shorten TestRun: create a helper (e.g., runTestCase or assertRunResult) that
accepts the deps produced by makeDeps, the test config (Config), the runner, and
the expected values, and moves the logic currently using captureStdout,
executeRun, comparing err vs tt.wantErr, comparing gotOutput vs tt.wantOutput,
and calling AssertExpectations for mockFSInstance, mockLog, and runner into that
helper; keep TestRun as a table-driven loop that only sets up deps via
makeDeps(makeDepsConfig{...}) and calls the new helper with executeRun,
captureStdout, and the tt expectations to bring TestRun back under the 50-line
limit while preserving use of executeRun and existing helpers.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/cli/cli_test.gointernal/fs/fs_test.gointernal/logger/logger.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/logger/logger.go
- Extract test execution logic into runTestCase helper function - Remove unused logger import and verbose log level configuration - Introduce testCase struct for standardized test definitions - Add nolint annotations for anonymous setup functions
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/cli/cli_test.go (1)
136-166:⚠️ Potential issue | 🟠 MajorAvoid re-implementing
Runlogic inexecuteRun.This helper duplicates the core flow, so
TestRuncan pass even ifRun(deps, config)regresses. Prefer a single production execution path (e.g., sharedrunWithRunner(...)) used by bothRunand tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 136 - 166, executeRun duplicates Run's main flow; extract the shared execution logic into a single helper (e.g., runWithRunner(deps Dependencies, config Config, runner ProgramRunner)) that encapsulates DetermineBinDir, RunTUI vs. binary removal, logging syncs and stdout messages, then have both Run and executeRun call runWithRunner instead of duplicating code; update executeRun to simply forward to runWithRunner(deps, config, runner) and remove the duplicated branches around deps.FS.DetermineBinDir, RunTUI, AdjustBinaryPath, RemoveBinary and log.Sync calls so tests and production use the same implementation.
🧹 Nitpick comments (1)
internal/cli/cli_test.go (1)
68-103: MakecaptureStdoutrestoration unconditional viat.Cleanup.
os.Stdoutis only restored when the returned closure is called. If a test exits early, global stdout and pipe FDs can leak into later tests. Register cleanup immediately after swapping stdout.Suggested hardening
func captureStdout(t *testing.T) func() string { t.Helper() oldStdout := os.Stdout r, w, err := os.Pipe() if err != nil { t.Fatalf("Failed to create pipe: %v", err) } os.Stdout = w + t.Cleanup(func() { + os.Stdout = oldStdout + _ = w.Close() + _ = r.Close() + }) return func() string { - os.Stdout = oldStdout if err := w.Close(); err != nil { t.Errorf("Failed to close pipe writer: %v", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cli/cli_test.go` around lines 68 - 103, captureStdout currently restores os.Stdout and closes pipe FDs only when the returned closure is invoked, which can leak stdout and FDs if a test exits early; update captureStdout to call t.Cleanup immediately after swapping os.Stdout to unconditionally restore os.Stdout and close the pipe reader/writer, and keep the returned closure for reading the captured output (ensure you reference the existing symbols r, w, oldStdout and use t.Cleanup to restore os.Stdout and close r/w even if the test fails or returns early).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/cli/cli_test.go`:
- Around line 136-166: executeRun duplicates Run's main flow; extract the shared
execution logic into a single helper (e.g., runWithRunner(deps Dependencies,
config Config, runner ProgramRunner)) that encapsulates DetermineBinDir, RunTUI
vs. binary removal, logging syncs and stdout messages, then have both Run and
executeRun call runWithRunner instead of duplicating code; update executeRun to
simply forward to runWithRunner(deps, config, runner) and remove the duplicated
branches around deps.FS.DetermineBinDir, RunTUI, AdjustBinaryPath, RemoveBinary
and log.Sync calls so tests and production use the same implementation.
---
Nitpick comments:
In `@internal/cli/cli_test.go`:
- Around line 68-103: captureStdout currently restores os.Stdout and closes pipe
FDs only when the returned closure is invoked, which can leak stdout and FDs if
a test exits early; update captureStdout to call t.Cleanup immediately after
swapping os.Stdout to unconditionally restore os.Stdout and close the pipe
reader/writer, and keep the returned closure for reading the captured output
(ensure you reference the existing symbols r, w, oldStdout and use t.Cleanup to
restore os.Stdout and close r/w even if the test fails or returns early).
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.
Migrate the logging infrastructure from zap to zerolog for improved performance and a simpler API. This change updates the logger interface, refactors the root command initialization, and updates all related tests and mocks.
Problem
The current logging implementation uses
go.uber.org/zapwhich requires a SugaredLogger wrapper and verbose configuration for log level management. The zap-based approach adds complexity to the codebase with its need for zapcore level conversions and WithOptions pattern for dynamic level changes.Solution
Replace zap with
github.com/rs/zerolog, a high-performance, zero-allocation JSON logger. The new implementation provides:Changes
Summary by CodeRabbit
New Features
Chores