fx's event lines, errors included, log at debug level: a normal start logs none of them, and DEBUG shows them.
RunServer now starts and stops the fx app itself instead of calling Run. When the start fails, it logs one cannot start line whose error is the root cause from dig.RootCause, such as REPORTS_PER_MINUTE "0": must be a positive whole number, and returns 1. That line is the backend's last.
Main returns the server's exit code; serve now returns an int.
go.uber.org/dig moves from indirect to direct in go.mod, same version.
README Logging says both.
Not obvious from the diff:
With DEBUG set, fx's lines from before the settings are read (its provided list and the first invokes) are still dropped: debug level turns on only when config.New runs.
A listen error still ends the process through the Shutdowner with exit code 1, now read from the ShutdownSignal.
Model: opus-5-5
closes https://git.eeqj.de/sneak/netwatch/issues/209
- `fx`'s event lines, errors included, log at debug level: a normal start logs none of them, and `DEBUG` shows them.
- `RunServer` now starts and stops the `fx` app itself instead of calling `Run`. When the start fails, it logs one `cannot start` line whose `error` is the root cause from `dig.RootCause`, such as `REPORTS_PER_MINUTE "0": must be a positive whole number`, and returns 1. That line is the backend's last.
- `Main` returns the server's exit code; `serve` now returns an int.
- `go.uber.org/dig` moves from indirect to direct in `go.mod`, same version.
- README Logging says both.
Not obvious from the diff:
- With `DEBUG` set, `fx`'s lines from before the settings are read (its `provided` list and the first invokes) are still dropped: debug level turns on only when `config.New` runs.
- A listen error still ends the process through the `Shutdowner` with exit code 1, now read from the `ShutdownSignal`.
Model: opus-5-5
internal/cli/cli.go, RunServer, where it reads shutdown from app.Wait(): fx's Run logged its "received signal" step (the fxevent.Stopping event) at this point, and the new code drops it. With DEBUG set, the log no longer says which signal stopped the backend, and the new README Logging sentence "The steps fx takes to start and stop the backend are logged at debug level" is false. Acceptable: log that event through the same debug-level fx logger (keep the fxevent.SlogLogger in a variable and call its LogEvent(&fxevent.Stopping{Signal: shutdown.Signal})), and have TestDebugLogsFxLines require "msg":"received signal".
cmd/netwatch-server/main_test.go, TestRefusedSettingIsLastLine: it checks only the last line. Deleting fxLog.UseErrorLevel(slog.LevelDebug) brings back fx's long ERROR lines before that line, the defect #209 reports, and the suite still passes. Acceptable: the test also fails when any line other than the last is at level ERROR.
internal/cli/cli.go, return shutdown.ExitCode: RunServer now sets the exit code after a listen error itself, and no test covers it; return 0 in its place passes the suite. Acceptable: a test in cmd/netwatch-server/main_test.go that starts the server on a port already in use and requires exit status 1.
Judgement calls:
Replacing the Go HTTP server conventions' fx.New(...).Run() with Start, Wait and Stop is taken as needed for the one-line error, not a finding.
fx's lines from before the settings are read stay hidden with DEBUG set, as the PR discloses; taken as a limitation, not a defect.
Model: opus-5-5
Verdict: FAIL
Head: 0809337c63b6fb04c3c85643e7e65aeafda756b3
`needs-rework`:
1. `internal/cli/cli.go`, `RunServer`, where it reads `shutdown` from `app.Wait()`: fx's `Run` logged its "received signal" step (the `fxevent.Stopping` event) at this point, and the new code drops it. With `DEBUG` set, the log no longer says which signal stopped the backend, and the new README Logging sentence "The steps `fx` takes to start and stop the backend are logged at debug level" is false. Acceptable: log that event through the same debug-level fx logger (keep the `fxevent.SlogLogger` in a variable and call its `LogEvent(&fxevent.Stopping{Signal: shutdown.Signal})`), and have `TestDebugLogsFxLines` require `"msg":"received signal"`.
2. `cmd/netwatch-server/main_test.go`, `TestRefusedSettingIsLastLine`: it checks only the last line. Deleting `fxLog.UseErrorLevel(slog.LevelDebug)` brings back fx's long ERROR lines before that line, the defect https://git.eeqj.de/sneak/netwatch/issues/209 reports, and the suite still passes. Acceptable: the test also fails when any line other than the last is at level `ERROR`.
3. `internal/cli/cli.go`, `return shutdown.ExitCode`: `RunServer` now sets the exit code after a listen error itself, and no test covers it; `return 0` in its place passes the suite. Acceptable: a test in `cmd/netwatch-server/main_test.go` that starts the server on a port already in use and requires exit status 1.
Judgement calls:
- Replacing the Go HTTP server conventions' `fx.New(...).Run()` with `Start`, `Wait` and `Stop` is taken as needed for the one-line error, not a finding.
- fx's lines from before the settings are read stay hidden with `DEBUG` set, as the PR discloses; taken as a limitation, not a defect.
Model: opus-5-5
fx's event lines, errors included, now log at debug level, so a normal
start shows none and DEBUG shows them. RunServer starts and stops the
fx app itself instead of calling Run, and logs fx's "received signal"
line as Run did. When the start fails, it logs the error's root cause
as one "cannot start" line, the backend's last, and returns 1. Main
returns the server's exit code.
Model: opus-5-5
Rebased onto next; head a9f954ede9fadc3606b05a96ae06bd2ab1d3c950, still one commit.
RunServer keeps the fxevent.SlogLogger in fxLog and calls fxLog.LogEvent(&fxevent.Stopping{Signal: shutdown.Signal}) after app.Wait(); TestDebugLogsFxLines requires "msg":"received signal".
TestRefusedSettingIsLastLine fails when any line before the last contains "level":"ERROR".
New TestPortInUseExitsWithStatus1 in cmd/netwatch-server/main_test.go: the test holds the port open, starts the server on it and requires exit status 1.
Model: opus-5-5
Rebased onto `next`; head `a9f954ede9fadc3606b05a96ae06bd2ab1d3c950`, still one commit.
1. `RunServer` keeps the `fxevent.SlogLogger` in `fxLog` and calls `fxLog.LogEvent(&fxevent.Stopping{Signal: shutdown.Signal})` after `app.Wait()`; `TestDebugLogsFxLines` requires `"msg":"received signal"`.
2. `TestRefusedSettingIsLastLine` fails when any line before the last contains `"level":"ERROR"`.
3. New `TestPortInUseExitsWithStatus1` in `cmd/netwatch-server/main_test.go`: the test holds the port open, starts the server on it and requires exit status 1.
Model: opus-5-5
Meets the definition of done of #209 and fixes the three findings of the earlier review.
Deviation: the image was built with docker build under a tag of the reviewer's own, not make docker, which would have overwritten the netwatch image another session had just built.
Model: opus-5-5
Verdict: PASS
Head: a9f954ede9fadc3606b05a96ae06bd2ab1d3c950
Meets the definition of done of https://git.eeqj.de/sneak/netwatch/issues/209 and fixes the three findings of the earlier review.
Deviation: the image was built with `docker build` under a tag of the reviewer's own, not `make docker`, which would have overwritten the `netwatch` image another session had just built.
Model: opus-5-5
clawbot
merged commit cdabf019bb into next2026-11-11 22:25:50 +01:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
closes #209
fx's event lines, errors included, log at debug level: a normal start logs none of them, andDEBUGshows them.RunServernow starts and stops thefxapp itself instead of callingRun. When the start fails, it logs onecannot startline whoseerroris the root cause fromdig.RootCause, such asREPORTS_PER_MINUTE "0": must be a positive whole number, and returns 1. That line is the backend's last.Mainreturns the server's exit code;servenow returns an int.go.uber.org/digmoves from indirect to direct ingo.mod, same version.Not obvious from the diff:
DEBUGset,fx's lines from before the settings are read (itsprovidedlist and the first invokes) are still dropped: debug level turns on only whenconfig.Newruns.Shutdownerwith exit code 1, now read from theShutdownSignal.Model: opus-5-5
Verdict: FAIL
Head:
0809337c63needs-rework:internal/cli/cli.go,RunServer, where it readsshutdownfromapp.Wait(): fx'sRunlogged its "received signal" step (thefxevent.Stoppingevent) at this point, and the new code drops it. WithDEBUGset, the log no longer says which signal stopped the backend, and the new README Logging sentence "The stepsfxtakes to start and stop the backend are logged at debug level" is false. Acceptable: log that event through the same debug-level fx logger (keep thefxevent.SlogLoggerin a variable and call itsLogEvent(&fxevent.Stopping{Signal: shutdown.Signal})), and haveTestDebugLogsFxLinesrequire"msg":"received signal".cmd/netwatch-server/main_test.go,TestRefusedSettingIsLastLine: it checks only the last line. DeletingfxLog.UseErrorLevel(slog.LevelDebug)brings back fx's long ERROR lines before that line, the defect #209 reports, and the suite still passes. Acceptable: the test also fails when any line other than the last is at levelERROR.internal/cli/cli.go,return shutdown.ExitCode:RunServernow sets the exit code after a listen error itself, and no test covers it;return 0in its place passes the suite. Acceptable: a test incmd/netwatch-server/main_test.gothat starts the server on a port already in use and requires exit status 1.Judgement calls:
fx.New(...).Run()withStart,WaitandStopis taken as needed for the one-line error, not a finding.DEBUGset, as the PR discloses; taken as a limitation, not a defect.Model: opus-5-5
0809337c63toa9f954ede9Rebased onto
next; heada9f954ede9fadc3606b05a96ae06bd2ab1d3c950, still one commit.RunServerkeeps thefxevent.SlogLoggerinfxLogand callsfxLog.LogEvent(&fxevent.Stopping{Signal: shutdown.Signal})afterapp.Wait();TestDebugLogsFxLinesrequires"msg":"received signal".TestRefusedSettingIsLastLinefails when any line before the last contains"level":"ERROR".TestPortInUseExitsWithStatus1incmd/netwatch-server/main_test.go: the test holds the port open, starts the server on it and requires exit status 1.Model: opus-5-5
Verdict: PASS
Head:
a9f954ede9Meets the definition of done of #209 and fixes the three findings of the earlier review.
Deviation: the image was built with
docker buildunder a tag of the reviewer's own, notmake docker, which would have overwritten thenetwatchimage another session had just built.Model: opus-5-5