Two configuration paths still failed silently, against the rule every other variable follows.
SENTRY_DSN
Parsed in loadFromEnv by a new envSentryDSN, shaped like envPort/envBindAddress and wrapping a new ErrInvalidSentryDSN. It parses with sentry.NewDsn, which is the call sentry.Init makes on the DSN it is handed, so configuration and initialisation cannot disagree about what a valid DSN is.
The trade-off, stated:internal/config now imports the Sentry SDK. It is already a module dependency already linked into this binary, so it costs nothing at build time. The alternative — a syntactic check in the config package — is a second definition of "valid DSN" free to drift from the one that actually decides. I took the import.
hasSentryDSN is replaced by Config.SentryEnabled(), following MetricsAuthEnabled(): one method read by the startup log field (now sentryEnabled), by the SDK initialisation, and — through s.sentryEnabled — by the sentryhttp middleware registration. The log cannot report reporting as on while nothing is sending.
enableSentry's error branch is fatal, comment and all, and Run gives up before it listens rather than binding a port it is about to release. Fatal means what a listen failure already meant here: Shutdowner.Shutdown(fx.ExitCode(1)) through fx's normal stop sequence — not a panic, not a bare os.Exit, and every stop hook still runs. shutdownOnListenFailure is therefore now shutdownWithFailure and ListenFailureExitCode is StartupFailureExitCode, since both now describe more than a listen.
Worth knowing for review: with config validating via NewDsn, that error branch is unreachable in this SDK version — sentry.NewClient in v0.25.0 returns an error only from NewDsn. It stays because that is a property of the SDK's current implementation, not of its contract. The test reaches it by putting an unparseable DSN on a hand-built Config, which bypasses loadFromEnv.
.env
The godotenv/autoload blank import is replaced by config.LoadDotEnv(). autoload discarded Load's error, and godotenv parses the whole file before setting anything — so one mistyped line applied none of it, reverting every variable in the file to its default with no log line naming the file.
A missing file stays fine: it is optional and most deployments have none. Only a file that is there and cannot be read or parsed aborts, naming it. A variable already in the real environment still wins over the file.
Scope disclosure: this needed cmd/webhooker/main.go too, which is outside the fence I was given. LoadDotEnv() is called at the top of dispatch(). autoload ran in an init(), ahead of config.DataDir() — which both the DATA_DIR lock in run() and resetpw call outside the fx graph. Loading only inside loadFromEnv() would move it after the lock, so a .env setting DATA_DIR would lock one directory while the config opened databases in another, and resetpw would never see the file at all. dispatch() is the one point ahead of every reader of the environment on both subcommand paths. The main.go change is 8 lines plus a helpCommand constant that goconst demanded once a fourth test used the literal.
Verification
make check green with GOFLAGS=-count=1 after the rebase onto current next (48cf93e). Lint ran uncached in Docker (0 issues, 56.6s, no CACHED on that layer); no test line reported (cached).
Both defects were reproduced against a build of unmodified next first, then re-checked against the built binary from this branch.
case
before
after
SENTRY_DSN=not-a-dsn
served, logged sentry init failure and "hasSentryDSN":true
started on default port 8080, file unmentioned in any log line
exit 1, one stderr line naming .env, before any log output and before the DATA_DIR lock
missing .env
starts
starts
valid .env
applied
applied — a DATA_DIR set only in .env produced webhooker.lock and webhooker.db in that directory, proving the load still precedes the lock
In the rejected-DSN state no sentryEnabled field is emitted at all, because the process never reaches the startup summary. resetpw is refused by a malformed .env on the same terms as the server.
Tests added: TestEnvSentryDSN (10 rows), TestSentryEnabled_TracksTheDSN, four SENTRY_DSN rows in the existing config.New table plus the unset case in TestNewUsesDefaultsWhenUnset, six TestLoadDotEnv_* covering missing / valid / real-environment-wins / malformed / unreadable / working-directory-relative, three TestDispatch_* pinning that .env is read before any subcommand runs, and TestSentryInitFailure_ShutsDownTheApp, which asserts the non-zero exit, that the stop sequence still completes, and that the port was never bound.
badEnvValueCases was split into three per-variable groups; the new rows pushed it past the funlen budget.
README
SENTRY_DSN added to the "Invalid values abort startup" enumeration, which omitted it, and the table row now says what unset and unparseable each do. The section's unqualified claim is left as it was — the code is now true instead. The .env paragraph no longer names godotenv/autoload and documents the parse behaviour: optional, malformed aborts, real environment wins.
Not done
TODO.md untouched, per instructions.
Closes https://git.eeqj.de/sneak/webhooker/issues/283.
Two configuration paths still failed silently, against the rule every other variable follows.
## SENTRY_DSN
Parsed in `loadFromEnv` by a new `envSentryDSN`, shaped like `envPort`/`envBindAddress` and wrapping a new `ErrInvalidSentryDSN`. It parses with `sentry.NewDsn`, which is the call `sentry.Init` makes on the DSN it is handed, so configuration and initialisation cannot disagree about what a valid DSN is.
**The trade-off, stated:** `internal/config` now imports the Sentry SDK. It is already a module dependency already linked into this binary, so it costs nothing at build time. The alternative — a syntactic check in the config package — is a second definition of "valid DSN" free to drift from the one that actually decides. I took the import.
`hasSentryDSN` is replaced by `Config.SentryEnabled()`, following `MetricsAuthEnabled()`: one method read by the startup log field (now `sentryEnabled`), by the SDK initialisation, and — through `s.sentryEnabled` — by the `sentryhttp` middleware registration. The log cannot report reporting as on while nothing is sending.
`enableSentry`'s error branch is fatal, comment and all, and `Run` gives up before it listens rather than binding a port it is about to release. Fatal means what a listen failure already meant here: `Shutdowner.Shutdown(fx.ExitCode(1))` through fx's normal stop sequence — not a panic, not a bare `os.Exit`, and every stop hook still runs. `shutdownOnListenFailure` is therefore now `shutdownWithFailure` and `ListenFailureExitCode` is `StartupFailureExitCode`, since both now describe more than a listen.
Worth knowing for review: with config validating via `NewDsn`, that error branch is **unreachable in this SDK version** — `sentry.NewClient` in v0.25.0 returns an error only from `NewDsn`. It stays because that is a property of the SDK's current implementation, not of its contract. The test reaches it by putting an unparseable DSN on a hand-built `Config`, which bypasses `loadFromEnv`.
## .env
The `godotenv/autoload` blank import is replaced by `config.LoadDotEnv()`. `autoload` discarded `Load`'s error, and godotenv parses the whole file before setting anything — so one mistyped line applied none of it, reverting every variable in the file to its default with no log line naming the file.
A **missing** file stays fine: it is optional and most deployments have none. Only a file that is there and cannot be read or parsed aborts, naming it. A variable already in the real environment still wins over the file.
**Scope disclosure:** this needed `cmd/webhooker/main.go` too, which is outside the fence I was given. `LoadDotEnv()` is called at the top of `dispatch()`. `autoload` ran in an `init()`, ahead of `config.DataDir()` — which both the `DATA_DIR` lock in `run()` and `resetpw` call outside the fx graph. Loading only inside `loadFromEnv()` would move it after the lock, so a `.env` setting `DATA_DIR` would lock one directory while the config opened databases in another, and `resetpw` would never see the file at all. `dispatch()` is the one point ahead of every reader of the environment on both subcommand paths. The `main.go` change is 8 lines plus a `helpCommand` constant that `goconst` demanded once a fourth test used the literal.
## Verification
`make check` green with `GOFLAGS=-count=1` after the rebase onto current `next` (48cf93e). Lint ran uncached in Docker (`0 issues`, 56.6s, no `CACHED` on that layer); no test line reported `(cached)`.
Both defects were reproduced against a build of unmodified `next` first, then re-checked against the built binary from this branch.
| case | before | after |
| --- | --- | --- |
| `SENTRY_DSN=not-a-dsn` | served, logged `sentry init failure` and `"hasSentryDSN":true` | exit 1, `invalid Sentry DSN: SENTRY_DSN: "not-a-dsn": [Sentry] DsnParseError: invalid scheme` |
| `SENTRY_DSN=%%%` | same | exit 1, names the variable and the value |
| `SENTRY_DSN=https://example.invalid/1` | same | exit 1, names the variable and the value |
| valid DSN | starts | starts, `"sentryEnabled":true`, `sentry error reporting activated` |
| `SENTRY_DSN` absent | starts | starts, `"sentryEnabled":false`, listener up |
| malformed `.env` | started on default port 8080, file unmentioned in any log line | exit 1, one stderr line naming `.env`, before any log output and before the `DATA_DIR` lock |
| missing `.env` | starts | starts |
| valid `.env` | applied | applied — a `DATA_DIR` set only in `.env` produced `webhooker.lock` and `webhooker.db` in that directory, proving the load still precedes the lock |
In the rejected-DSN state no `sentryEnabled` field is emitted at all, because the process never reaches the startup summary. `resetpw` is refused by a malformed `.env` on the same terms as the server.
Tests added: `TestEnvSentryDSN` (10 rows), `TestSentryEnabled_TracksTheDSN`, four `SENTRY_DSN` rows in the existing `config.New` table plus the unset case in `TestNewUsesDefaultsWhenUnset`, six `TestLoadDotEnv_*` covering missing / valid / real-environment-wins / malformed / unreadable / working-directory-relative, three `TestDispatch_*` pinning that `.env` is read before any subcommand runs, and `TestSentryInitFailure_ShutsDownTheApp`, which asserts the non-zero exit, that the stop sequence still completes, and that the port was never bound.
`badEnvValueCases` was split into three per-variable groups; the new rows pushed it past the `funlen` budget.
## README
`SENTRY_DSN` added to the "Invalid values abort startup" enumeration, which omitted it, and the table row now says what unset and unparseable each do. The section's unqualified claim is left as it was — the code is now true instead. The `.env` paragraph no longer names `godotenv/autoload` and documents the parse behaviour: optional, malformed aborts, real environment wins.
## Not done
`TODO.md` untouched, per instructions.
Two configuration paths still failed silently, against the rule every
other variable follows: a value that is set but cannot be parsed must
abort startup rather than substitute a default.
SENTRY_DSN is now parsed in loadFromEnv, with sentry.NewDsn — the same
call sentry.Init makes on the DSN it is handed, so configuration and
initialisation cannot disagree about what a valid DSN is. That costs
internal/config an import of the Sentry SDK, which is already a module
dependency already linked into the binary, and buys a single definition
of validity rather than a hand-rolled second one free to drift. A typo
in a DSN used to log one error line and leave the process serving with
error reporting off forever, which nothing downstream can notice: the
variable is still set, so every later signal reports it as on.
hasSentryDSN is replaced by Config.SentryEnabled(), following
MetricsAuthEnabled(): one method read by the startup log field, by the
SDK initialisation and by the sentryhttp middleware, so the log cannot
report reporting as on while nothing is sending. enableSentry's error
branch is now fatal, and Run gives up before it listens rather than
binding a port it is about to release. Fatal there means what a listen
failure already meant — Shutdowner.Shutdown(fx.ExitCode(1)), through
fx's normal stop sequence — so shutdownOnListenFailure is now
shutdownWithFailure and ListenFailureExitCode is
StartupFailureExitCode.
The godotenv/autoload blank import is replaced by config.LoadDotEnv,
called at the top of dispatch. autoload discarded Load's error, and
godotenv applies nothing at all when a file will not parse, so one
mistyped line reverted every variable in the file to its default and
started the server with no log line naming the file. A missing file
stays fine — it is optional and most deployments have none. The call
sits in dispatch rather than in loadFromEnv because autoload ran in an
init(), ahead of config.DataDir(), which both the DATA_DIR lock and
resetpw call outside the fx graph; loading any later would let a .env
that sets DATA_DIR lock one directory while the config opened
databases in another.
Both defects were reproduced against the previous build first: an
unparseable DSN served traffic while logging "hasSentryDSN":true, and
a malformed .env started on the default port with the file unmentioned.
PASS. Both defects fixed against the iron rule; verified by execution, not by reading.
Anomalies/disclosures, none blocking:
help now exits 1 on a malformed .env. Because LoadDotEnv() sits at the top of dispatch(), webhooker help (and an unknown subcommand) is refused when .env will not parse — the one subcommand that reads no config. It is deliberate, documented in the TestDispatch_MalformedDotEnvRefuses comment, and loud rather than silent, so it is not a defect; flagging it as a behaviour change a reader might not expect.
Precedence is stated but worth restating for operators: a variable already present in the real environment wins over .env (godotenv.Load does not overwrite). Verified: .envPORT=19713 with a real PORT=19714 bound 127.0.0.1:19714. README documents this.
Probe disclosure: the reviewed tree was never modified. Runtime probes and three mutation tests ran against binaries built in a throwaway copy outside the clone; make check / make fmt-check ran in the reviewed tree via the make targets only, lint in Docker.
Judgement calls raised to sneak rather than filed here: the Sentry SDK import into internal/config, and keeping enableSentry's error branch that is unreachable in sentry-go v0.25.0.
**PASS.** Both defects fixed against the iron rule; verified by execution, not by reading.
Anomalies/disclosures, none blocking:
- **`help` now exits 1 on a malformed `.env`.** Because `LoadDotEnv()` sits at the top of `dispatch()`, `webhooker help` (and an unknown subcommand) is refused when `.env` will not parse — the one subcommand that reads no config. It is deliberate, documented in the `TestDispatch_MalformedDotEnvRefuses` comment, and loud rather than silent, so it is not a defect; flagging it as a behaviour change a reader might not expect.
- **Precedence is stated but worth restating for operators:** a variable already present in the real environment wins over `.env` (`godotenv.Load` does not overwrite). Verified: `.env` `PORT=19713` with a real `PORT=19714` bound `127.0.0.1:19714`. README documents this.
- **Probe disclosure:** the reviewed tree was never modified. Runtime probes and three mutation tests ran against binaries built in a throwaway copy outside the clone; `make check` / `make fmt-check` ran in the reviewed tree via the `make` targets only, lint in Docker.
- Judgement calls raised to sneak rather than filed here: the Sentry SDK import into `internal/config`, and keeping `enableSentry`'s error branch that is unreachable in sentry-go v0.25.0.
clawbot
merged commit b9f7db6901 into next2026-08-24 04:25:30 +02: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 #283.
Two configuration paths still failed silently, against the rule every other variable follows.
SENTRY_DSN
Parsed in
loadFromEnvby a newenvSentryDSN, shaped likeenvPort/envBindAddressand wrapping a newErrInvalidSentryDSN. It parses withsentry.NewDsn, which is the callsentry.Initmakes on the DSN it is handed, so configuration and initialisation cannot disagree about what a valid DSN is.The trade-off, stated:
internal/confignow imports the Sentry SDK. It is already a module dependency already linked into this binary, so it costs nothing at build time. The alternative — a syntactic check in the config package — is a second definition of "valid DSN" free to drift from the one that actually decides. I took the import.hasSentryDSNis replaced byConfig.SentryEnabled(), followingMetricsAuthEnabled(): one method read by the startup log field (nowsentryEnabled), by the SDK initialisation, and — throughs.sentryEnabled— by thesentryhttpmiddleware registration. The log cannot report reporting as on while nothing is sending.enableSentry's error branch is fatal, comment and all, andRungives up before it listens rather than binding a port it is about to release. Fatal means what a listen failure already meant here:Shutdowner.Shutdown(fx.ExitCode(1))through fx's normal stop sequence — not a panic, not a bareos.Exit, and every stop hook still runs.shutdownOnListenFailureis therefore nowshutdownWithFailureandListenFailureExitCodeisStartupFailureExitCode, since both now describe more than a listen.Worth knowing for review: with config validating via
NewDsn, that error branch is unreachable in this SDK version —sentry.NewClientin v0.25.0 returns an error only fromNewDsn. It stays because that is a property of the SDK's current implementation, not of its contract. The test reaches it by putting an unparseable DSN on a hand-builtConfig, which bypassesloadFromEnv..env
The
godotenv/autoloadblank import is replaced byconfig.LoadDotEnv().autoloaddiscardedLoad's error, and godotenv parses the whole file before setting anything — so one mistyped line applied none of it, reverting every variable in the file to its default with no log line naming the file.A missing file stays fine: it is optional and most deployments have none. Only a file that is there and cannot be read or parsed aborts, naming it. A variable already in the real environment still wins over the file.
Scope disclosure: this needed
cmd/webhooker/main.gotoo, which is outside the fence I was given.LoadDotEnv()is called at the top ofdispatch().autoloadran in aninit(), ahead ofconfig.DataDir()— which both theDATA_DIRlock inrun()andresetpwcall outside the fx graph. Loading only insideloadFromEnv()would move it after the lock, so a.envsettingDATA_DIRwould lock one directory while the config opened databases in another, andresetpwwould never see the file at all.dispatch()is the one point ahead of every reader of the environment on both subcommand paths. Themain.gochange is 8 lines plus ahelpCommandconstant thatgoconstdemanded once a fourth test used the literal.Verification
make checkgreen withGOFLAGS=-count=1after the rebase onto currentnext(48cf93e). Lint ran uncached in Docker (0 issues, 56.6s, noCACHEDon that layer); no test line reported(cached).Both defects were reproduced against a build of unmodified
nextfirst, then re-checked against the built binary from this branch.SENTRY_DSN=not-a-dsnsentry init failureand"hasSentryDSN":trueinvalid Sentry DSN: SENTRY_DSN: "not-a-dsn": [Sentry] DsnParseError: invalid schemeSENTRY_DSN=%%%SENTRY_DSN=https://example.invalid/1"sentryEnabled":true,sentry error reporting activatedSENTRY_DSNabsent"sentryEnabled":false, listener up.env.env, before any log output and before theDATA_DIRlock.env.envDATA_DIRset only in.envproducedwebhooker.lockandwebhooker.dbin that directory, proving the load still precedes the lockIn the rejected-DSN state no
sentryEnabledfield is emitted at all, because the process never reaches the startup summary.resetpwis refused by a malformed.envon the same terms as the server.Tests added:
TestEnvSentryDSN(10 rows),TestSentryEnabled_TracksTheDSN, fourSENTRY_DSNrows in the existingconfig.Newtable plus the unset case inTestNewUsesDefaultsWhenUnset, sixTestLoadDotEnv_*covering missing / valid / real-environment-wins / malformed / unreadable / working-directory-relative, threeTestDispatch_*pinning that.envis read before any subcommand runs, andTestSentryInitFailure_ShutsDownTheApp, which asserts the non-zero exit, that the stop sequence still completes, and that the port was never bound.badEnvValueCaseswas split into three per-variable groups; the new rows pushed it past thefunlenbudget.README
SENTRY_DSNadded to the "Invalid values abort startup" enumeration, which omitted it, and the table row now says what unset and unparseable each do. The section's unqualified claim is left as it was — the code is now true instead. The.envparagraph no longer namesgodotenv/autoloadand documents the parse behaviour: optional, malformed aborts, real environment wins.Not done
TODO.mduntouched, per instructions.PASS. Both defects fixed against the iron rule; verified by execution, not by reading.
Anomalies/disclosures, none blocking:
helpnow exits 1 on a malformed.env. BecauseLoadDotEnv()sits at the top ofdispatch(),webhooker help(and an unknown subcommand) is refused when.envwill not parse — the one subcommand that reads no config. It is deliberate, documented in theTestDispatch_MalformedDotEnvRefusescomment, and loud rather than silent, so it is not a defect; flagging it as a behaviour change a reader might not expect..env(godotenv.Loaddoes not overwrite). Verified:.envPORT=19713with a realPORT=19714bound127.0.0.1:19714. README documents this.make check/make fmt-checkran in the reviewed tree via themaketargets only, lint in Docker.internal/config, and keepingenableSentry's error branch that is unreachable in sentry-go v0.25.0.