sneak/webhooker
1
0
Fork 0
Code Issues 24 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

Fail loudly on set-but-unparseable env config values (closes #80) #92

Open
clawbot wants to merge 1 commits from issue-80-config-fail-loud into main
pull from: issue-80-config-fail-loud
merge into: sneak:main
Conversation 4 Commits 1 Files Changed 5 +597 -49
clawbot commented 2026-08-09 03:56:55 +02:00
Collaborator
Copy Link

Closes #80. Single commit on top of main @ 4f5ecb1.

Why

The config env helpers silently substituted the documented default
whenever a variable was set but could not be parsed. A typo in an
operator-supplied value therefore produced a running daemon with
configuration nobody asked for, instead of a startup failure:
PORT=eighty quietly listened on 8080, and DEBUG=ture (or yes, or
on) quietly disabled debug logging because envBool treated every
non-true/1 value as false.

The policy this PR encodes: a set-but-invalid environment value
aborts startup; defaults apply only to keys that are unset or
empty. There is no third behaviour.

This is an intentional behaviour change. An existing deployment with a
malformed config value will now fail to start rather than run with a
silently substituted default.

What changed

internal/config/config.go:

  • envPositiveInt added verbatim from PR #87. Same name, same
    signature envPositiveInt(key string, defaultValue int) (int, error),
    same package-level ErrNonPositiveValue, same error strings
    (invalid integer for %s: %q: %w and
    %w: %s must be at least 1, got %q). See the note on #87 below.
  • envInt removed entirely. Its only caller was PORT, which is
    now parsed by a new envPort wrapper: envPositiveInt covers
    unset/unparseable/<1, and envPort adds the TCP upper bound with a
    new package-level ErrInvalidPort (maxPort = 65535). No
    silent-fallback integer variant survives in the package.
  • envBool now returns (bool, error) and parses with
    strconv.ParseBool rather than a hand-rolled spelling table. Unset or
    empty keeps the default; anything else that fails to parse is a
    wrapped error naming the key and the value. Callers are DEBUG and
    MAINTENANCE_MODE. This deliberately narrows the accepted set:
    yes, on, and mixed-case oddities that strings.EqualFold used to
    swallow are now startup errors.
  • Propagation. Env loading moved into loadFromEnv, which returns
    the first error it hits; config.New returns it so fx aborts startup.
    Every error names the offending key and value. The
    WEBHOOKER_ENVIRONMENT block was extracted into resolveEnvironment
    to keep New inside the funlen budget — also copied verbatim from
    #87, for the same rebase reason.

envString and envDuration were audited

Explicitly, so the reviewer does not have to re-derive it:

  • envString is a bare os.Getenv passthrough. It parses nothing, so
    there is no set-but-unparseable case to fail on. Unchanged.
  • envDuration was already made fail-loud in #78: unset returns the
    default, a set-but-unparseable value returns
    invalid duration for %s: %q: %w. Unchanged.

Repo-wide os.Getenv audit (spec step 5)

git ls-files piped through a grep for os.Getenv and os.LookupEnv
across every tracked file in the repo returns exactly one file:
internal/config/config.go. (Untracked sibling worktrees under
.claude/ show up in a naive recursive grep; they are copies of this
same file, not additional call sites.)

There are no environment-variable parse sites outside
internal/config, so there was nothing else to fix under this issue.
cmd/webhooker and every internal/* package take their configuration
from the injected *config.Config.

Relationship to PR #87 (unmerged, merge-ready)

PR #87 (issue-64-receiver-rate-limit) introduces an identical
envPositiveInt and ErrNonPositiveValue, plus the same
resolveEnvironment extraction. That duplication is deliberate and was
specified in the issue: whichever of the two lands first, rebasing the
other is a delete-one-copy operation on those definitions rather
than a semantic merge. The copies here are byte-for-byte identical to
#87's, comments included, so a rebase should produce no behavioural
question at all. The remaining conflict surface is the usual TODO.md
and README churn.

Tests

internal/config/env_test.go (new), with the unexported helpers reached
through a small export_test.go shim so each helper gets its own table
without widening the package API:

  • TestEnvBool — unset (both defaults), empty, true/1 /
    False/0, and rejection of yes, on, ture.
  • TestEnvPositiveInt — unset, empty, 42, unparseable, 0, -5
    (the last two asserted with errors.Is(err, ErrNonPositiveValue)).
  • TestEnvPort — unset, 9000, 65535, unparseable, 0, and 65536
    asserted with errors.Is(err, ErrInvalidPort).
  • TestNewRejectsBadEnvValues — config.New through fx: a bad PORT
    (unparseable and out-of-range), a bad DEBUG, and a bad
    MAINTENANCE_MODE each abort with an error naming the key and the
    value, while valid values are applied.
  • TestNewUsesDefaultsWhenUnset — the legitimate unset case still
    yields port 8080 and both booleans false.

All env manipulation uses t.Setenv (with os.Unsetenv for the
deliberately-absent cases), so nothing leaks between tests.

Docs

  • README.md: new "Invalid values abort startup" subsection under the
    configuration table stating that defaults apply only to unset
    variables, that a set-but-unparseable value aborts startup, that
    PORT must be 1–65535, and listing the exact boolean spellings
    strconv.ParseBool accepts. MAINTENANCE_MODE added to the table
    since this PR changes how it parses.
  • TODO.md: updated in the same commit, with the stale Next Step (the
    retention reaper, delivered in #63) rotated into Completed Steps.

Verification

  • make fmt then make check — tests and fmt-check green.
  • script/cibuild (Docker, golangci-lint v2.12.2 as pinned in the
    Dockerfile) — exit 0, so lint is green under the pinned
    toolchain that CI uses.
  • Note for the reviewer: a local make lint on the host reports one
    gosec G704 in internal/delivery/client_ssrf_test.go. That is
    pre-existing host/CI version skew — the host golangci-lint is v2.10.1
    and reports it on pristine origin/main too; the pinned v2.12.2 does
    not. Nothing to do with this change.
  • .golangci.yml untouched (sha256 unchanged,
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb);
    the pinned golangci-lint v2.12.2 Dockerfile digest is unchanged.
Closes #80. Single commit on top of `main` @ `4f5ecb1`. ## Why The config env helpers silently substituted the documented default whenever a variable was set but could not be parsed. A typo in an operator-supplied value therefore produced a running daemon with configuration nobody asked for, instead of a startup failure: `PORT=eighty` quietly listened on 8080, and `DEBUG=ture` (or `yes`, or `on`) quietly disabled debug logging because `envBool` treated every non-`true`/`1` value as false. The policy this PR encodes: **a set-but-invalid environment value aborts startup**; defaults apply **only** to keys that are unset or empty. There is no third behaviour. This is an intentional behaviour change. An existing deployment with a malformed config value will now fail to start rather than run with a silently substituted default. ## What changed `internal/config/config.go`: - **`envPositiveInt` added verbatim from PR #87.** Same name, same signature `envPositiveInt(key string, defaultValue int) (int, error)`, same package-level `ErrNonPositiveValue`, same error strings (`invalid integer for %s: %q: %w` and `%w: %s must be at least 1, got %q`). See the note on #87 below. - **`envInt` removed entirely.** Its only caller was `PORT`, which is now parsed by a new `envPort` wrapper: `envPositiveInt` covers unset/unparseable/&lt;1, and `envPort` adds the TCP upper bound with a new package-level `ErrInvalidPort` (`maxPort = 65535`). No silent-fallback integer variant survives in the package. - **`envBool` now returns `(bool, error)`** and parses with `strconv.ParseBool` rather than a hand-rolled spelling table. Unset or empty keeps the default; anything else that fails to parse is a wrapped error naming the key and the value. Callers are `DEBUG` and `MAINTENANCE_MODE`. This deliberately narrows the accepted set: `yes`, `on`, and mixed-case oddities that `strings.EqualFold` used to swallow are now startup errors. - **Propagation.** Env loading moved into `loadFromEnv`, which returns the first error it hits; `config.New` returns it so fx aborts startup. Every error names the offending key and value. The `WEBHOOKER_ENVIRONMENT` block was extracted into `resolveEnvironment` to keep `New` inside the `funlen` budget — also copied verbatim from #87, for the same rebase reason. ## `envString` and `envDuration` were audited Explicitly, so the reviewer does not have to re-derive it: - `envString` is a bare `os.Getenv` passthrough. It parses nothing, so there is no set-but-unparseable case to fail on. Unchanged. - `envDuration` was already made fail-loud in #78: unset returns the default, a set-but-unparseable value returns `invalid duration for %s: %q: %w`. Unchanged. ## Repo-wide `os.Getenv` audit (spec step 5) `git ls-files` piped through a grep for `os.Getenv` and `os.LookupEnv` across every tracked file in the repo returns exactly one file: `internal/config/config.go`. (Untracked sibling worktrees under `.claude/` show up in a naive recursive grep; they are copies of this same file, not additional call sites.) **There are no environment-variable parse sites outside `internal/config`**, so there was nothing else to fix under this issue. `cmd/webhooker` and every `internal/*` package take their configuration from the injected `*config.Config`. ## Relationship to PR #87 (unmerged, merge-ready) PR #87 (`issue-64-receiver-rate-limit`) introduces an **identical** `envPositiveInt` and `ErrNonPositiveValue`, plus the same `resolveEnvironment` extraction. That duplication is deliberate and was specified in the issue: whichever of the two lands first, rebasing the other is a **delete-one-copy** operation on those definitions rather than a semantic merge. The copies here are byte-for-byte identical to #87's, comments included, so a rebase should produce no behavioural question at all. The remaining conflict surface is the usual `TODO.md` and README churn. ## Tests `internal/config/env_test.go` (new), with the unexported helpers reached through a small `export_test.go` shim so each helper gets its own table without widening the package API: - `TestEnvBool` — unset (both defaults), empty, `true`/`1` / `False`/`0`, and rejection of `yes`, `on`, `ture`. - `TestEnvPositiveInt` — unset, empty, `42`, unparseable, `0`, `-5` (the last two asserted with `errors.Is(err, ErrNonPositiveValue)`). - `TestEnvPort` — unset, `9000`, `65535`, unparseable, `0`, and `65536` asserted with `errors.Is(err, ErrInvalidPort)`. - `TestNewRejectsBadEnvValues` — `config.New` through fx: a bad `PORT` (unparseable and out-of-range), a bad `DEBUG`, and a bad `MAINTENANCE_MODE` each abort with an error naming the key and the value, while valid values are applied. - `TestNewUsesDefaultsWhenUnset` — the legitimate unset case still yields port 8080 and both booleans false. All env manipulation uses `t.Setenv` (with `os.Unsetenv` for the deliberately-absent cases), so nothing leaks between tests. ## Docs - `README.md`: new "Invalid values abort startup" subsection under the configuration table stating that defaults apply only to unset variables, that a set-but-unparseable value aborts startup, that `PORT` must be 1–65535, and listing the exact boolean spellings `strconv.ParseBool` accepts. `MAINTENANCE_MODE` added to the table since this PR changes how it parses. - `TODO.md`: updated in the same commit, with the stale Next Step (the retention reaper, delivered in #63) rotated into Completed Steps. ## Verification - `make fmt` then `make check` — tests and `fmt-check` green. - `script/cibuild` (Docker, golangci-lint **v2.12.2** as pinned in the `Dockerfile`) — **exit 0**, so lint is green under the pinned toolchain that CI uses. - Note for the reviewer: a local `make lint` on the host reports one `gosec` G704 in `internal/delivery/client_ssrf_test.go`. That is pre-existing host/CI version skew — the host golangci-lint is v2.10.1 and reports it on pristine `origin/main` too; the pinned v2.12.2 does not. Nothing to do with this change. - `.golangci.yml` untouched (sha256 unchanged, `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`); the pinned golangci-lint v2.12.2 Dockerfile digest is unchanged.
clawbot added the needs-review label 2026-08-09 03:56:55 +02:00
clawbot added 1 commit 2026-08-09 03:56:55 +02:00
Fail loudly on set-but-unparseable env config values (closes #80)
All checks were successful
check / check (push) Successful in 6m3s
Details
985464dcf9 
The config env helpers silently substituted the documented default
whenever a variable was set but could not be parsed, so a typo in an
operator-supplied value produced a running daemon with configuration
nobody asked for instead of a startup failure. `PORT=eighty` quietly
listened on 8080 and `DEBUG=ture` quietly disabled debug logging.

Defaults now apply only to variables that are unset or empty. Any
variable that is set but unparseable is a hard error that names the
key and the offending value and aborts startup through fx.

- add `envPositiveInt` with `ErrNonPositiveValue`, copied verbatim
  from the definition on the unmerged #87 so that rebasing after it
  lands is a delete-one-copy operation rather than a semantic merge
- remove `envInt` entirely; `PORT` is parsed by a new `envPort`,
  which adds the TCP upper bound (`ErrInvalidPort`, 1..65535)
- change `envBool` to return an error and parse with
  `strconv.ParseBool`, so `yes`, `on`, and typos are rejected rather
  than silently treated as false; callers are `DEBUG` and
  `MAINTENANCE_MODE`
- move env loading into `loadFromEnv`, with the environment check
  extracted to `resolveEnvironment` (also as #87 defines it), keeping
  `New` within the funlen budget

`envString` parses nothing and `envDuration` was already fail-loud,
so both are unchanged. A repo-wide audit of `os.Getenv`/`os.LookupEnv`
found no parse sites outside `internal/config`.

Tests cover each helper with a table (unset, valid, set-but-invalid)
plus `config.New`-level cases proving a bad `PORT`, `DEBUG`, or
`MAINTENANCE_MODE` aborts startup while unset variables still get
their defaults. README documents the fail-loud rule and the accepted
boolean spellings.
clawbot self-assigned this 2026-08-09 03:56:59 +02:00
clawbot commented 2026-08-09 03:57:16 +02:00
Author
Collaborator
Copy Link

Summary of what was built and how it was verified

One commit, 985464d, on issue-80-config-fail-loud off main @
4f5ecb1. 5 files, +597/-49.

Behaviour delivered: no helper in internal/config falls back to a
default for a value the operator actually set. envInt is gone;
PORT goes through envPort (envPositiveInt for
unset/unparseable/<1, plus ErrInvalidPort above 65535); envBool
returns an error and uses strconv.ParseBool. config.New propagates
the first error through loadFromEnv, so fx aborts startup and every
message names the offending key and value. An unset variable still
gets its documented default — covered by an explicit test.

Audit results, so the reviewer need not redo them: envString
parses nothing (bare os.Getenv) and envDuration was already
fail-loud from #78; both intentionally unchanged. A grep for
os.Getenv/os.LookupEnv over every tracked file returns only
internal/config/config.go — there are no parse sites elsewhere in the
repo, so nothing outside internal/config needed fixing.

Verification (repo entrypoints only, no raw go or
golangci-lint):

  • make fmt, then make check — 172 tests pass, fmt-check clean.
  • script/cibuild — exit 0. This is the authoritative lint gate: it
    builds through the Dockerfile, whose lint stage is pinned to
    golangci-lint v2.12.2 by digest, the same toolchain CI uses. Two
    earlier iterations were red under it (funlen, paralleltest,
    then 6 goconst) and were fixed; the run on 985464d's tree is
    green.
  • Only findings not addressed: one pre-existing gosec G704 in
    internal/delivery/client_ssrf_test.go, which the host
    golangci-lint v2.10.1 reports on pristine origin/main as well and
    the pinned v2.12.2 does not report at all. Out of scope here, and
    not introduced by this change.
  • .golangci.yml untouched; sha256 still
    021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.
    The pinned lint image digest in Dockerfile is unchanged.

Rebase note: envPositiveInt, ErrNonPositiveValue, and
resolveEnvironment are byte-for-byte copies of the definitions on
the unmerged, merge-ready #87. Whichever lands first, the other's
rebase is a delete-one-copy edit on those definitions plus the usual
TODO.md/README.md churn.

## Summary of what was built and how it was verified One commit, `985464d`, on `issue-80-config-fail-loud` off `main` @ `4f5ecb1`. 5 files, +597/-49. **Behaviour delivered**: no helper in `internal/config` falls back to a default for a value the operator actually set. `envInt` is gone; `PORT` goes through `envPort` (`envPositiveInt` for unset/unparseable/&lt;1, plus `ErrInvalidPort` above 65535); `envBool` returns an error and uses `strconv.ParseBool`. `config.New` propagates the first error through `loadFromEnv`, so fx aborts startup and every message names the offending key and value. An unset variable still gets its documented default — covered by an explicit test. **Audit results, so the reviewer need not redo them**: `envString` parses nothing (bare `os.Getenv`) and `envDuration` was already fail-loud from #78; both intentionally unchanged. A grep for `os.Getenv`/`os.LookupEnv` over every tracked file returns only `internal/config/config.go` — there are no parse sites elsewhere in the repo, so nothing outside `internal/config` needed fixing. **Verification** (repo entrypoints only, no raw `go` or `golangci-lint`): - `make fmt`, then `make check` — 172 tests pass, `fmt-check` clean. - `script/cibuild` — exit 0. This is the authoritative lint gate: it builds through the `Dockerfile`, whose lint stage is pinned to golangci-lint v2.12.2 by digest, the same toolchain CI uses. Two earlier iterations were red under it (`funlen`, `paralleltest`, then 6 `goconst`) and were fixed; the run on `985464d`'s tree is green. - Only findings not addressed: one pre-existing `gosec` G704 in `internal/delivery/client_ssrf_test.go`, which the *host* golangci-lint v2.10.1 reports on pristine `origin/main` as well and the pinned v2.12.2 does not report at all. Out of scope here, and not introduced by this change. - `.golangci.yml` untouched; sha256 still `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`. The pinned lint image digest in `Dockerfile` is unchanged. **Rebase note**: `envPositiveInt`, `ErrNonPositiveValue`, and `resolveEnvironment` are byte-for-byte copies of the definitions on the unmerged, merge-ready #87. Whichever lands first, the other's rebase is a delete-one-copy edit on those definitions plus the usual `TODO.md`/`README.md` churn.
clawbot commented 2026-08-09 04:06:59 +02:00
Author
Collaborator
Copy Link

Review of PR #92 @ 985464d

Verdict: PASS. No blocking findings. Eight non-blocking nits are
listed below; none of them need to be fixed before merge.

Verified by execution

  • script/cibuild on the PR head tree — exit 0. Runs the pinned
    golangci-lint:v2.12.2@sha256:5cceeef0… lint stage, make fmt-check,
    make test, and make build.
  • Gitea check / check (push) on 985464d — success in 6m3s
    (run 98). It was still queued at the start of this review and has
    since completed green.
  • make check on the host — every test passes. It exits non-zero only
    on internal/delivery/client_ssrf_test.go:78 G704 (gosec). That
    file is not in this diff, and this is the documented host
    golangci-lint v2.10.1 / pinned v2.12.2 skew. Not attributable to this
    change.
  • make fmt — no drift; git status --porcelain clean afterwards.
    make check modifies no tracked files (REPO_POLICIES line 233).
  • Mutation test (in a scratch copy outside the review worktree, no
    repo files touched): reintroduced silent defaulting in three places —
    envBool returning defaultValue, nil on ParseBool failure, the
    i &lt; 1 guard removed from envPositiveInt, and the
    port &gt; maxPort guard removed from envPort. Result: 11 subtest
    failures across TestEnvBool, TestEnvPositiveInt, TestEnvPort,
    and TestNewRejectsBadEnvValues. The tests are not vacuous; every
    reintroduction of the defect is caught.
  • PR #87 fidelity: extracted envPositiveInt and
    resolveEnvironment from both origin/issue-64-receiver-rate-limit
    and this head and diffed them — byte-for-byte identical, comments
    included. ErrNonPositiveValue declaration and doc comment likewise
    identical. Error strings match the spec exactly
    (invalid integer for %s: %q: %w and
    %w: %s must be at least 1, got %q). No divergence found.
  • envPort bounds: 1 and 65535 accepted, 0 / -5 rejected via
    ErrNonPositiveValue, 65536 rejected via ErrInvalidPort, all
    confirmed by passing subtests. No off-by-one. Overflowing inputs
    (99999999999999999999) and whitespace-padded inputs fail through
    strconv.Atoi rather than defaulting.
  • git merge-tree --write-tree origin/main HEAD — merges clean against
    main @ 4f5ecb1, which is also the PR base. Not stale.
  • sha256sum .golangci.yml = 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb,
    matching the required value; git diff origin/main..HEAD is empty for
    .golangci.yml, Dockerfile, go.mod, and go.sum, so the
    v2.12.2 digest pin is intact.

Verified by reading

  • Core policy holds. Every parse site in internal/config was read,
    not just the changed ones. resolveEnvironment (unset -> dev,
    unrecognised -> ErrInvalidEnvironment), envBool, envPositiveInt,
    envPort, and envDuration all return an error on a set-but-invalid
    value. envString is a bare os.Getenv and parses nothing. There is
    no remaining code path in the package that substitutes a default for
    a value the operator set. envInt is gone from the package entirely.
  • Empty-string handling is deliberate and documented: PORT="",
    DEBUG="" etc. take the default, and README.md says so explicitly
    ("unset (or set to an empty string)"). This matches the spec's
    "unset (or empty)" wording. Correct call — an empty value in a
    compose/systemd environment is conventionally an absent value.
  • Error propagation is complete. loadFromEnv (config.go:221-261)
    performs five fallible reads and each is followed by
    if err != nil { return nil, err }. Nothing is logged-and-continued,
    nothing is assigned to _. New (config.go:266-306) returns the
    error before any logging or defaulting, so fx aborts. Both envBool
    callers (DEBUG, MAINTENANCE_MODE) propagate.
  • envBool narrowing uses strconv.ParseBool per the library
    policy, and the README lists the accepted spellings accurately
    (1 t T TRUE true True 0 f F FALSE false False) and states that
    yes, on, and off are now rejected. The mixed-case narrowing
    (TrUe, previously accepted by strings.EqualFold) is stated as
    intentional in the commit message, the PR body, and the README. No
    file tracked in this repo (no compose file, unit file, or .env)
    sets a now-rejected spelling, so nothing in-tree breaks.
  • os.Getenv audit independently reproduced. git grep for
    os.Getenv, os.LookupEnv, and syscall.Getenv over tracked files
    returns exactly five hits, all in internal/config/config.go. The
    PR body's claim is accurate; there are no parse sites elsewhere.
  • Scope discipline. Five files, +597/-49, all within the issue's
    remit. The loadFromEnv extraction is behaviour-preserving: the same
    values are read, the struct is populated identically, and log /
    params are assigned in New afterwards. The only observable
    difference is which error wins when two variables are simultaneously
    bad (PORT now precedes RETENTION_SWEEP_INTERVAL), which is not
    meaningful. export_test.go is test-only and does not widen the
    shipped API.
  • Hygiene. Single commit; subject
    Fail loudly on set-but-unparseable env config values (closes #80)
    ends with the required (closes #80). Author and committer are
    clawbot. No Co-Authored-By, no session trailers, and a
    case-insensitive grep for Claude/Anthropic over the whole tree
    returns nothing. No 4-byte UTF-8 anywhere in the diff. TODO.md is
    updated in the same commit. No non-inclusive terminology introduced.
    Test-file idiom (t.Setenv, the t.Parallel explanatory comment,
    require/assert split, table-per-helper) matches the existing
    internal/config/config_test.go exactly.

Non-blocking nits

  1. internal/config/env_test.go:112, 189, 270, 399 — the "unset" cases
    call os.Unsetenv with no t.Cleanup restore, so the unset state
    leaks for the rest of the test binary. TestNewUsesDefaultsWhenUnset
    permanently unsets the real PORT, DEBUG, and MAINTENANCE_MODE.
    Harmless today (config_test.go sorts before env_test.go, and this
    is the last test in the file) and it is exactly the pre-existing
    idiom in config_test.go:59, 160, 231, so consistency argues for
    leaving it. If it is ever cleaned up, do both files together with a
    save/restore helper registered through t.Cleanup.
  2. internal/config/env_test.go:314-388 — TestNewRejectsBadEnvValues
    sets only the one key under test and does not neutralise ambient
    PORT / DEBUG / MAINTENANCE_MODE / DATA_DIR /
    RETENTION_SWEEP_INTERVAL. A developer with any of those exported in
    their shell can get a spurious failure. Again matches the existing
    pattern. Acceptable would be unsetting the full set at the top of
    each subtest before applying tt.key.
  3. internal/config/env_test.go:277-280 — the TestEnvPort error
    branch asserts the message contains the key but, unlike its two
    sibling tables, omits assert.Contains(t, err.Error(), tt.value).
    Both envPort error paths do include the value
    (got "0" and got 65536), so the assertion would hold; dropping it
    makes this table weaker than the others for no reason.
  4. internal/config/config.go:47-49 — ErrInvalidPort reads as "any
    invalid port" but covers only the above-range case; PORT=0 yields
    ErrNonPositiveValue, so errors.Is(err, ErrInvalidPort) is false
    for a value most operators would call an invalid port. The doc
    comment does say "set above the valid port range", and the issue spec
    asked for this name specifically, so this is fine as shipped —
    ErrPortOutOfRange would just be less surprising.
  5. internal/config/config.go:158-168 — envPort renders the offending
    value with %d while envPositiveInt two functions above renders it
    with %q. Cosmetic inconsistency between adjacent error strings.
    envPositiveInt's form is locked by the verbatim-copy requirement,
    so only envPort could move.
  6. internal/config/config.go:94-98 — the envBool godoc says "Returns
    defaultValue if not set" without mentioning that an empty string is
    also treated as unset, which is the behaviour the README documents
    and the tests assert. envPositiveInt has the same gap but its
    wording is frozen by the #87 copy requirement; envBool's is not.
  7. README.md:104 — the new prose cites
    RETENTION_SWEEP_INTERVAL=1 hour as a fail-loud example, but
    RETENTION_SWEEP_INTERVAL is not a row in the configuration table
    directly above it. That omission predates this PR (#78 added the
    variable without a table row), but this change makes it visible.
    Adding the row would be a one-line follow-up.
  8. internal/config/export_test.go — the conventional Go shim is
    var EnvBool = envBool rather than a wrapper named
    EnvBoolForTest; the ForTest suffix is redundant in a file that
    only exists during test builds. Purely stylistic, and the current
    form is perfectly readable.

Note for the #87 rebase (not a defect in this PR)

issue-64-receiver-rate-limit still carries the old silent-fallback
envInt alongside its copy of envPositiveInt. If #92 lands first, the
#87 rebase must drop both its duplicate envPositiveInt /
ErrNonPositiveValue / resolveEnvironment definitions and its
envInt; a mechanical conflict resolution that keeps envInt would
silently reintroduce exactly the defect #80 fixes. Worth flagging on #87
so whoever rebases it is looking for that.

# Review of PR #92 @ `985464d` **Verdict: PASS.** No blocking findings. Eight non-blocking nits are listed below; none of them need to be fixed before merge. ## Verified by execution - `script/cibuild` on the PR head tree — **exit 0**. Runs the pinned `golangci-lint:v2.12.2@sha256:5cceeef0…` lint stage, `make fmt-check`, `make test`, and `make build`. - Gitea `check / check (push)` on `985464d` — **success in 6m3s** (run 98). It was still queued at the start of this review and has since completed green. - `make check` on the host — every test passes. It exits non-zero only on `internal/delivery/client_ssrf_test.go:78` `G704` (gosec). That file is not in this diff, and this is the documented host golangci-lint v2.10.1 / pinned v2.12.2 skew. Not attributable to this change. - `make fmt` — no drift; `git status --porcelain` clean afterwards. `make check` modifies no tracked files (REPO_POLICIES line 233). - **Mutation test** (in a scratch copy outside the review worktree, no repo files touched): reintroduced silent defaulting in three places — `envBool` returning `defaultValue, nil` on `ParseBool` failure, the `i &lt; 1` guard removed from `envPositiveInt`, and the `port &gt; maxPort` guard removed from `envPort`. Result: 11 subtest failures across `TestEnvBool`, `TestEnvPositiveInt`, `TestEnvPort`, and `TestNewRejectsBadEnvValues`. The tests are **not** vacuous; every reintroduction of the defect is caught. - **PR #87 fidelity**: extracted `envPositiveInt` and `resolveEnvironment` from both `origin/issue-64-receiver-rate-limit` and this head and diffed them — **byte-for-byte identical**, comments included. `ErrNonPositiveValue` declaration and doc comment likewise identical. Error strings match the spec exactly (`invalid integer for %s: %q: %w` and `%w: %s must be at least 1, got %q`). No divergence found. - **`envPort` bounds**: 1 and 65535 accepted, 0 / -5 rejected via `ErrNonPositiveValue`, 65536 rejected via `ErrInvalidPort`, all confirmed by passing subtests. No off-by-one. Overflowing inputs (`99999999999999999999`) and whitespace-padded inputs fail through `strconv.Atoi` rather than defaulting. - `git merge-tree --write-tree origin/main HEAD` — merges clean against `main` @ `4f5ecb1`, which is also the PR base. Not stale. - `sha256sum .golangci.yml` = `021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb`, matching the required value; `git diff origin/main..HEAD` is empty for `.golangci.yml`, `Dockerfile`, `go.mod`, and `go.sum`, so the v2.12.2 digest pin is intact. ## Verified by reading - **Core policy holds.** Every parse site in `internal/config` was read, not just the changed ones. `resolveEnvironment` (unset -&gt; dev, unrecognised -&gt; `ErrInvalidEnvironment`), `envBool`, `envPositiveInt`, `envPort`, and `envDuration` all return an error on a set-but-invalid value. `envString` is a bare `os.Getenv` and parses nothing. There is no remaining code path in the package that substitutes a default for a value the operator set. `envInt` is gone from the package entirely. - **Empty-string handling** is deliberate and documented: `PORT=""`, `DEBUG=""` etc. take the default, and `README.md` says so explicitly ("unset (or set to an empty string)"). This matches the spec's "unset (or empty)" wording. Correct call — an empty value in a compose/systemd environment is conventionally an absent value. - **Error propagation is complete.** `loadFromEnv` (config.go:221-261) performs five fallible reads and each is followed by `if err != nil { return nil, err }`. Nothing is logged-and-continued, nothing is assigned to `_`. `New` (config.go:266-306) returns the error before any logging or defaulting, so fx aborts. Both `envBool` callers (`DEBUG`, `MAINTENANCE_MODE`) propagate. - **`envBool` narrowing** uses `strconv.ParseBool` per the library policy, and the README lists the accepted spellings accurately (`1 t T TRUE true True 0 f F FALSE false False`) and states that `yes`, `on`, and `off` are now rejected. The mixed-case narrowing (`TrUe`, previously accepted by `strings.EqualFold`) is stated as intentional in the commit message, the PR body, and the README. No file tracked in this repo (no compose file, unit file, or `.env`) sets a now-rejected spelling, so nothing in-tree breaks. - **`os.Getenv` audit independently reproduced.** `git grep` for `os.Getenv`, `os.LookupEnv`, and `syscall.Getenv` over tracked files returns exactly five hits, all in `internal/config/config.go`. The PR body's claim is accurate; there are no parse sites elsewhere. - **Scope discipline.** Five files, +597/-49, all within the issue's remit. The `loadFromEnv` extraction is behaviour-preserving: the same values are read, the struct is populated identically, and `log` / `params` are assigned in `New` afterwards. The only observable difference is which error wins when two variables are simultaneously bad (`PORT` now precedes `RETENTION_SWEEP_INTERVAL`), which is not meaningful. `export_test.go` is test-only and does not widen the shipped API. - **Hygiene.** Single commit; subject `Fail loudly on set-but-unparseable env config values (closes #80)` ends with the required ` (closes #80)`. Author and committer are `clawbot`. No `Co-Authored-By`, no session trailers, and a case-insensitive grep for Claude/Anthropic over the whole tree returns nothing. No 4-byte UTF-8 anywhere in the diff. `TODO.md` is updated in the same commit. No non-inclusive terminology introduced. Test-file idiom (`t.Setenv`, the `t.Parallel` explanatory comment, `require`/`assert` split, table-per-helper) matches the existing `internal/config/config_test.go` exactly. ## Non-blocking nits 1. `internal/config/env_test.go:112, 189, 270, 399` — the "unset" cases call `os.Unsetenv` with no `t.Cleanup` restore, so the unset state leaks for the rest of the test binary. `TestNewUsesDefaultsWhenUnset` permanently unsets the real `PORT`, `DEBUG`, and `MAINTENANCE_MODE`. Harmless today (`config_test.go` sorts before `env_test.go`, and this is the last test in the file) and it is exactly the pre-existing idiom in `config_test.go:59, 160, 231`, so consistency argues for leaving it. If it is ever cleaned up, do both files together with a save/restore helper registered through `t.Cleanup`. 2. `internal/config/env_test.go:314-388` — `TestNewRejectsBadEnvValues` sets only the one key under test and does not neutralise ambient `PORT` / `DEBUG` / `MAINTENANCE_MODE` / `DATA_DIR` / `RETENTION_SWEEP_INTERVAL`. A developer with any of those exported in their shell can get a spurious failure. Again matches the existing pattern. Acceptable would be unsetting the full set at the top of each subtest before applying `tt.key`. 3. `internal/config/env_test.go:277-280` — the `TestEnvPort` error branch asserts the message contains the key but, unlike its two sibling tables, omits `assert.Contains(t, err.Error(), tt.value)`. Both `envPort` error paths do include the value (`got "0"` and `got 65536`), so the assertion would hold; dropping it makes this table weaker than the others for no reason. 4. `internal/config/config.go:47-49` — `ErrInvalidPort` reads as "any invalid port" but covers only the above-range case; `PORT=0` yields `ErrNonPositiveValue`, so `errors.Is(err, ErrInvalidPort)` is false for a value most operators would call an invalid port. The doc comment does say "set above the valid port range", and the issue spec asked for this name specifically, so this is fine as shipped — `ErrPortOutOfRange` would just be less surprising. 5. `internal/config/config.go:158-168` — `envPort` renders the offending value with `%d` while `envPositiveInt` two functions above renders it with `%q`. Cosmetic inconsistency between adjacent error strings. `envPositiveInt`'s form is locked by the verbatim-copy requirement, so only `envPort` could move. 6. `internal/config/config.go:94-98` — the `envBool` godoc says "Returns defaultValue if not set" without mentioning that an empty string is also treated as unset, which is the behaviour the README documents and the tests assert. `envPositiveInt` has the same gap but its wording is frozen by the #87 copy requirement; `envBool`'s is not. 7. `README.md:104` — the new prose cites `RETENTION_SWEEP_INTERVAL=1 hour` as a fail-loud example, but `RETENTION_SWEEP_INTERVAL` is not a row in the configuration table directly above it. That omission predates this PR (#78 added the variable without a table row), but this change makes it visible. Adding the row would be a one-line follow-up. 8. `internal/config/export_test.go` — the conventional Go shim is `var EnvBool = envBool` rather than a wrapper named `EnvBoolForTest`; the `ForTest` suffix is redundant in a file that only exists during test builds. Purely stylistic, and the current form is perfectly readable. ## Note for the #87 rebase (not a defect in this PR) `issue-64-receiver-rate-limit` still carries the old silent-fallback `envInt` alongside its copy of `envPositiveInt`. If #92 lands first, the #87 rebase must drop **both** its duplicate `envPositiveInt` / `ErrNonPositiveValue` / `resolveEnvironment` definitions **and** its `envInt`; a mechanical conflict resolution that keeps `envInt` would silently reintroduce exactly the defect #80 fixes. Worth flagging on #87 so whoever rebases it is looking for that.
clawbot commented 2026-08-09 04:08:25 +02:00
Author
Collaborator
Copy Link

Manager note

Independent review verdict: PASS, no blocking findings. The reviewer did not author this change.

What raises my confidence here beyond a read-only review:

  • The reviewer mutation-tested the change in a scratch copy outside the review worktree: reintroducing silent defaulting in envBool, deleting the i &lt; 1 guard in envPositiveInt, and deleting the port &gt; maxPort guard in envPort produced 11 subtest failures. The tests genuinely pin the behaviour rather than passing incidentally — which is the main risk with a change of this shape, where it is easy to write tests that assert assert.Error and prove nothing.
  • The #87 fidelity requirement was verified mechanically, not by eye: envPositiveInt, resolveEnvironment, and ErrNonPositiveValue were extracted from both origin/issue-64-receiver-rate-limit and this branch and diffed. Byte-for-byte identical, comments included. That is exactly what the spec asked for and it means the rebase after #87 lands is a delete-one-copy operation.
  • The repo-wide os.Getenv audit claim was independently reproduced rather than taken on trust: five hits, all in internal/config/config.go.
  • envPort bounds were checked for off-by-one at both ends: 1 and 65535 accepted, 0 and -5 rejected, 65536 rejected.

Also confirmed: script/cibuild exit 0, Gitea CI green on 985464d (6m3s — the reviewer polled it to completion rather than assuming), merges clean against main @ 4f5ecb1, .golangci.yml sha256 unchanged, Dockerfile/go.mod/go.sum zero diff.

The eight non-blocking nits are tracked as #94 rather than round-tripping this PR — they are test-hygiene and documentation items, none of which affect the correctness of the fix.

Labeled merge-ready and assigned to @sneak.

Merge-ordering hazard, please read before merging

This PR and #87 both define envPositiveInt, ErrNonPositiveValue, and resolveEnvironment, and #87 also still carries the old silent-fallback envInt that this PR deletes.

If #92 lands first, the #87 rebase must delete both its duplicated helpers and its envInt. A mechanical conflict resolution that keeps envInt would silently reinstate the exact defect #80 exists to fix, and nothing would fail — envInt would simply sit there unused until someone wired a new variable through it. I have posted the same warning on #87.

PR #91 (#90) is also merge-ready and also touches TODO.md, so whichever of #91/#92 lands second needs a trivial TODO.md rebase.

## Manager note Independent review verdict: **PASS**, no blocking findings. The reviewer did not author this change. What raises my confidence here beyond a read-only review: - The reviewer **mutation-tested** the change in a scratch copy outside the review worktree: reintroducing silent defaulting in `envBool`, deleting the `i &lt; 1` guard in `envPositiveInt`, and deleting the `port &gt; maxPort` guard in `envPort` produced **11 subtest failures**. The tests genuinely pin the behaviour rather than passing incidentally — which is the main risk with a change of this shape, where it is easy to write tests that assert `assert.Error` and prove nothing. - The **#87 fidelity requirement was verified mechanically**, not by eye: `envPositiveInt`, `resolveEnvironment`, and `ErrNonPositiveValue` were extracted from both `origin/issue-64-receiver-rate-limit` and this branch and diffed. Byte-for-byte identical, comments included. That is exactly what the spec asked for and it means the rebase after #87 lands is a delete-one-copy operation. - The repo-wide `os.Getenv` audit claim was **independently reproduced** rather than taken on trust: five hits, all in `internal/config/config.go`. - `envPort` bounds were checked for off-by-one at both ends: 1 and 65535 accepted, 0 and -5 rejected, 65536 rejected. Also confirmed: `script/cibuild` exit 0, Gitea CI green on `985464d` (6m3s — the reviewer polled it to completion rather than assuming), merges clean against `main` @ `4f5ecb1`, `.golangci.yml` sha256 unchanged, `Dockerfile`/`go.mod`/`go.sum` zero diff. The eight non-blocking nits are tracked as #94 rather than round-tripping this PR — they are test-hygiene and documentation items, none of which affect the correctness of the fix. Labeled `merge-ready` and assigned to @sneak. ### Merge-ordering hazard, please read before merging This PR and #87 both define `envPositiveInt`, `ErrNonPositiveValue`, and `resolveEnvironment`, and #87 **also still carries the old silent-fallback `envInt`** that this PR deletes. If #92 lands first, the #87 rebase must delete **both** its duplicated helpers **and** its `envInt`. A mechanical conflict resolution that keeps `envInt` would silently reinstate the exact defect #80 exists to fix, and nothing would fail — `envInt` would simply sit there unused until someone wired a new variable through it. I have posted the same warning on #87. PR #91 (#90) is also merge-ready and also touches `TODO.md`, so whichever of #91/#92 lands second needs a trivial `TODO.md` rebase.
clawbot added merge-ready and removed needs-review labels 2026-08-09 04:08:30 +02:00
clawbot removed their assignment 2026-08-09 04:08:42 +02:00
sneak was assigned by clawbot 2026-08-09 04:08:42 +02:00
clawbot commented 2026-08-09 07:37:44 +02:00
Author
Collaborator
Copy Link

Verification re-check: the green is real

A fleet-wide warning came in that script/cibuild can report a green it did not earn — it is a plain docker build . with no cache control, and the Dockerfile does COPY . . then RUN make check, so on an unchanged tree Docker serves the check layer from cache and the build exits 0 without running anything. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer CACHED.

This PR's verification cites script/cibuild exit 0 as the authoritative lint evidence, and in this repo that claim carries real weight: the host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so a cached layer would leave the pinned-linter result entirely unproven.

Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:

  • 985464d — check / check (push): success in 6m3s (run 98)

A cached build finishes in under a second. Six minutes is a genuine execution inside the pinned v2.12.2 image. The claim stands. No re-label, no pull-back.

All five currently merge-ready PRs were re-checked the same way and all have genuine multi-minute CI runs: #87 2m37s, #91 3m6s, #92 6m3s, #96 2m43s, #100 3m3s.

## Verification re-check: the green is real A fleet-wide warning came in that `script/cibuild` can report a green it did not earn — it is a plain `docker build .` with no cache control, and the Dockerfile does `COPY . .` then `RUN make check`, so on an unchanged tree Docker serves the check layer from cache and the build exits 0 without running anything. Observed elsewhere as a SUCCESS in 0.262 seconds with every layer `CACHED`. This PR's verification cites `script/cibuild` exit 0 as the authoritative lint evidence, and in this repo that claim carries real weight: the host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, so a cached layer would leave the pinned-linter result entirely unproven. **Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:** - `985464d` — `check / check (push)`: **success in 6m3s** (run 98) A cached build finishes in under a second. Six minutes is a genuine execution inside the pinned v2.12.2 image. **The claim stands.** No re-label, no pull-back. All five currently merge-ready PRs were re-checked the same way and all have genuine multi-minute CI runs: #87 2m37s, #91 3m6s, #92 6m3s, #96 2m43s, #100 3m3s.
All checks were successful
check / check (push) Successful in 6m3s
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue-80-config-fail-loud:issue-80-config-fail-loud
git checkout issue-80-config-fail-loud
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label merge-ready
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees sneak
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#92
Delete Branch "issue-80-config-fail-loud"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?