feat(logging): introduce pkg/logging slog wrapper; wire into main#29
Merged
kevinelliott merged 2 commits intomainfrom Apr 21, 2026
Merged
feat(logging): introduce pkg/logging slog wrapper; wire into main#29kevinelliott merged 2 commits intomainfrom
kevinelliott merged 2 commits intomainfrom
Conversation
Adds a thin pkg/logging package that wraps log/slog with config-driven
setup so level, format, and file destination come from cfg.Logging
rather than being hardcoded at each call site.
pkg/logging/logging.go
- New(cfg): constructs a *slog.Logger from cfg.Logging.{Level,Format,File}.
Invalid level strings fall back to info (never block startup). Failure
to open cfg.Logging.File falls back to stderr with a single warning.
- WithContext / FromContext: context plumbing; nil-logger is a no-op and
nil-context falls back to slog.Default.
- Install: swaps slog.Default so third-party libraries and package-level
slog calls pick up the configured handler.
No custom Info/Warn/Error wrappers on purpose — slog is the stable API
and wrapping it only hides who's calling from grep.
Wired into:
- cmd/agentmgr/main.go: logging.Install(logging.New(cfg)).
- cmd/agentmgr-helper/main.go: same, plus the shutdown-signal fmt.Printf
becomes slog.Info (the helper is a long-running process where
structured logs are most useful).
- pkg/api/grpc/server.go: recovery interceptors now emit slog.Error with
method / panic / stack fields instead of log.Printf format strings.
Tests: 100% coverage of pkg/logging.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Introduces a new pkg/logging package that configures log/slog from cfg.Logging and installs it as the process-wide default logger so structured logging is consistent across binaries and libraries.
Changes:
- Add
pkg/loggingwrapper with config-driven level/format/destination plus context helpers andslog.Default()installation. - Wire logging installation into
cmd/agentmgrandcmd/agentmgr-helperstartup; convert helper shutdown signal output toslog.Info. - Replace gRPC panic recovery logging from
log.Printfto structuredslog.Errorentries (method/panic/stack).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/logging/logging.go | New slog configuration + context plumbing + default logger install |
| pkg/logging/logging_test.go | Tests covering level/format/file fallback/context/install behaviors |
| pkg/logging/logging_test_helpers.go | Helper to read files used by tests |
| pkg/api/grpc/server.go | Switch panic recovery logs to structured slog |
| cmd/agentmgr/main.go | Install configured slog as default at startup |
| cmd/agentmgr-helper/main.go | Install configured slog as default; log shutdown signal via slog |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2
to
+10
|
|
||
| import "os" | ||
|
|
||
| // openForRead is extracted so test code can open files without pulling | ||
| // os directly into the test file (keeps the test imports minimal and | ||
| // lets the helper be reused if more tests need file IO). | ||
| func openForRead(p string) (*os.File, error) { | ||
| return os.Open(p) | ||
| } |
There was a problem hiding this comment.
This helper file is named logging_test_helpers.go (not *_test.go), so it will be compiled into production builds of pkg/logging. If it’s intended only for tests, move it into a *_test.go file (or inline it) and use os.ReadFile/os.Open directly in the tests.
Suggested change
| import "os" | |
| // openForRead is extracted so test code can open files without pulling | |
| // os directly into the test file (keeps the test imports minimal and | |
| // lets the helper be reused if more tests need file IO). | |
| func openForRead(p string) (*os.File, error) { | |
| return os.Open(p) | |
| } |
Comment on lines
+41
to
+45
| // One level below `want` should NOT be enabled (unless we're at debug). | ||
| below := want - 4 | ||
| if want == slog.LevelDebug { | ||
| return | ||
| } |
There was a problem hiding this comment.
This test assumes slog level spacing is always 4 (below := want - 4). That’s an internal detail; using want-1 (or a small delta) still verifies “below threshold is disabled” without depending on specific level constants.
Suggested change
| // One level below `want` should NOT be enabled (unless we're at debug). | |
| below := want - 4 | |
| if want == slog.LevelDebug { | |
| return | |
| } | |
| // A level just below `want` should NOT be enabled (unless we're at debug). | |
| if want == slog.LevelDebug { | |
| return | |
| } | |
| below := want - 1 |
| } | ||
|
|
||
| func TestNew_JSONFormat(t *testing.T) { | ||
| // Capture output by routing through a pipe-backed file. |
There was a problem hiding this comment.
The comment says “pipe-backed file”, but the test is writing to a regular temp file path. Suggest updating the comment to reflect what’s actually happening (temp file written by New).
Suggested change
| // Capture output by routing through a pipe-backed file. | |
| // Capture output by having New write to a temporary file path. |
Comment on lines
+41
to
+42
| // A nil cfg or nil cfg.Logging returns a sensible default (info, text, | ||
| // stderr) rather than panicking — handy in tests and early startup. |
There was a problem hiding this comment.
The doc comment says “nil cfg.Logging” is handled, but Config.Logging is a value type (not a pointer) so it can’t be nil. Suggest adjusting the comment to just describe the nil cfg behavior (and/or the zero-value LoggingConfig behavior).
Suggested change
| // A nil cfg or nil cfg.Logging returns a sensible default (info, text, | |
| // stderr) rather than panicking — handy in tests and early startup. | |
| // A nil cfg returns a sensible default (info, text, stderr) rather than | |
| // panicking. Likewise, the zero value of cfg.Logging yields those defaults, | |
| // which is handy in tests and early startup. |
| func WithContext(ctx context.Context, logger *slog.Logger) context.Context { | ||
| if logger == nil { | ||
| return ctx | ||
| } |
There was a problem hiding this comment.
WithContext will panic if ctx is nil because context.WithValue rejects a nil parent context. If this helper is meant to be safe for nil contexts, guard ctx == nil (e.g., treat it as context.Background() or return a context carrying the logger) and add a test for this case.
Suggested change
| } | |
| } | |
| if ctx == nil { | |
| ctx = context.Background() | |
| } |
Comment on lines
+62
to
+66
| if cfg.Logging.File != "" { | ||
| f, err := os.OpenFile(cfg.Logging.File, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0o644) | ||
| if err == nil { | ||
| out = f | ||
| } else { |
There was a problem hiding this comment.
When cfg.Logging.File is set, New opens the log file and assigns the *os.File to the handler writer, but the file is never closed. This can leak file descriptors in tests (and makes clean shutdown/flush harder in binaries). Consider returning a cleanup/close function (or exposing a Close/Shutdown helper) so callers can close the underlying file when done.
Windows TempDir cleanup fails when a file handle is still open at test end (the slog file handler holds it for the logger's lifetime). Route format tests through a bytes.Buffer via an extracted buildHandler helper so no file IO happens in these tests; the file-open-failure fallback test still hits the real New() path. Also tightens log file permissions from 0o644 to 0o600 — logs may contain secrets/PII and don't need to be world-readable. Fixes gosec G302 on pkg/logging/logging.go. Drops the unused logging_test_helpers.go (tripped gosec G304; the tests it supported no longer need a file at all). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
kevinelliott
added a commit
that referenced
this pull request
Apr 21, 2026
Promote the Unreleased section to 1.2.0 and capture the UX / operability work landed since v1.1.0: - #20 install/update progress streaming with -v - #21 gRPC server wire-up in helper - #22, #26 test coverage (cli/output 0→82%, systray 0.2→3.2%) - #24 RefreshOnStart field removal - #27 bubbles v0.21 -> v1.0 - #28 go:embed baseline catalog (fresh `go install` works offline) - #29 pkg/logging slog wrapper wired into main and grpc recovery Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
kevinelliott
added a commit
that referenced
this pull request
Apr 25, 2026
* fix: address Copilot review feedback — correctness fixes Seven real-bug fixes called out across review comments on merged PRs: - pkg/platform/platform.go: resetLookPathCache() no longer reassigns the package-level sync.Map (unsafe vs concurrent readers). Uses Range+Delete instead. - pkg/installer/providers/brew.go: GetLatestVersion no longer caches context-canceled / deadline-exceeded errors. A caller with a short ctx would have poisoned the 5m TTL for every later caller. - pkg/installer/providers/progress.go: the progress writer is now wrapped in a locking writer so concurrent Write calls from cmd.Stdout and cmd.Stderr (os/exec writes those from separate goroutines) don't interleave bytes mid-call. - pkg/catalog/embed.go: EmbeddedJSON() now returns a copy of the backing bytes. Previously callers could mutate the package-level []byte, corrupting the baseline for every future Get(). - pkg/catalog/manager.go: loadEmbedded now also probes /usr/share/agentmgr/catalog.json — where the goreleaser nfpm config installs the catalog for .deb/.rpm users. Previously that path was silently ignored at runtime even though goreleaser wrote the file there at install time. - internal/cli/doctor.go: Catalog Sources section updated with the matching /usr/share/agentmgr row so `doctor` output mirrors the loader's probe order. - internal/cli/agent.go: removed the --all (-a) and --check-updates flags from `agent list`. Both bound to local vars that were never read anywhere, so the flags did nothing; users setting them got a silent no-op instead of a useful behavior. Supersedes the corresponding Copilot annotations on PRs #19, #28, #29, #33, plus the dead-flag note on the agent list command. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore: address remaining Copilot review nits (docs + tests) Batch of small review-comment fixes — documentation accuracy, test hardening, and gosec G104 on grpc teardown: - pkg/api/grpc/server.go: explicit \`_ = s.listener.Close()\` in Stop() and ForceStop() with a comment noting the close error is not actionable during teardown (silences gosec G104). - pkg/catalog/manager.go: Refresh doc now explicitly notes that singleflight does not merge contexts and the first caller's ctx governs the shared fetch — a caller with a short deadline can cancel the in-flight work for all shared callers. - pkg/installer/providers/progress.go: WithProgressWriter doc clarifies why passing nil is a no-op (asymmetry lets middleware attach a writer safely without downstream code accidentally clobbering it). - internal/systray/cli_uninstall_darwin.go: file is darwin-only via //go:build tag, so platform.ID() is always Darwin at runtime. Removed the dead Linux and Windows branches (the filepath/os/platform imports they needed are gone too). - CHANGELOG.md: "(Removed in [Unreleased].)" was a broken shortcut reference link — promoted to "(Removed in v1.2.0.)". - README.md: feature bullet uses the full "agentmgr agent list" command prefix, consistent with everywhere else in the doc. - internal/cli/output/spinner_test.go: non-TTY test replaced the 30ms sleep with a direct assertion on s.isTTY. Start() is a no-op when isTTY is false, so sleeping was measuring nothing. - internal/cli/output/table_test.go: ANSI-alignment loop now actually asserts inter-column padding instead of discarding each line. - internal/systray/handlers_test.go: mustParseVersion panics on parse error so test-setup bugs surface at the fixture, not downstream. TestDialogTracking uses bare *exec.Cmd values (trackDialog only checks pointer identity) to avoid the PATH lookup for \`true\` on Windows CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(cli): drop "all" flag assertion from list; assert removal The --all / -a and --check-updates flags were removed from \`agent list\` in the parent commit because both bound to unused local vars. Drop the existence assertion for "all" and add an anti-regression check that asserts both flags are ABSENT. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Introduces a thin `pkg/logging` package wrapping `log/slog` that is driven by `cfg.Logging`. Level, format (text/json), and destination (stderr or a file) all come from config rather than being hardcoded per call site. Installs itself as `slog.Default()` from each binary's `main` so libraries and package-level slog calls also pick up the configuration.
API
No custom `Info/Warn/Error` wrappers — slog is the stable stdlib API, and wrapping it just hides callers from grep.
Wired sites
Deliberate non-goals
The CLI user-output calls (`fmt.Fprintln(os.Stderr, "Updating %s...")`, spinners, table renderers) are intentionally not migrated — those are UI output, not logs. Only genuine log sinks get the structured treatment in this PR.
Resilience
Tests
Test plan
🤖 Generated with Claude Code