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

Merged

clawbot merged 1 commits from issue-80-config-fail-loud into next 2026-08-10 16:06:12 +02:00

Conversation 4 Commits 1 Files Changed 5

+596 -48

clawbot commented 2026-08-09 03:56:55 +02:00

Collaborator

Copy Link

Copy Source

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 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

Copy Source

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 referenced this pull request 2026-08-09 04:03:46 +02:00

Enforce the body size limit before CSRF parses the form (closes #90) #91

clawbot referenced this pull request 2026-08-09 04:05:21 +02:00

Archive writer lifecycle: evict writers on webhook deletion and sweep idle archives #89

clawbot commented 2026-08-09 04:06:59 +02:00

Author

Collaborator

Copy Link

Copy Source

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 < 1 guard removed from envPositiveInt, and the
  port > 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 referenced this pull request 2026-08-09 04:08:12 +02:00

Follow-ups from the PR #92 review: config test hygiene and env docs #94

clawbot commented 2026-08-09 04:08:25 +02:00

Author

Collaborator

Copy Link

Copy Source

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 < 1 guard in envPositiveInt, and deleting the port > 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 referenced this pull request 2026-08-09 04:08:38 +02:00

Rate-limit the public webhook receiver endpoint (closes #64) #87

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 referenced this pull request 2026-08-09 04:26:30 +02:00

Evict archive writers on deletion and sweep idle archives (closes #89) #95

clawbot referenced this pull request 2026-08-09 05:01:01 +02:00

Deprecated gomodguard linter in the org-standard .golangci.yml (needs an upstream decision) #98

clawbot referenced this pull request 2026-08-09 07:37:40 +02:00

Root background loops at context.Background() (closes #97) #100

clawbot commented 2026-08-09 07:37:44 +02:00

Author

Collaborator

Copy Link

Copy Source

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.

clawbot referenced this pull request 2026-08-09 07:37:52 +02:00

Enforce the body size limit before CSRF parses the form (closes #90) #91

clawbot referenced this pull request 2026-08-09 07:37:59 +02:00

Allow retention_days of 0 to mean retain forever (closes #79) #96

clawbot referenced this pull request 2026-08-09 07:46:12 +02:00

Add inactivity-based session timeout #66

clawbot referenced this pull request 2026-08-09 07:50:41 +02:00

Add inactivity-based session timeout #66

clawbot referenced this pull request 2026-08-09 08:05:33 +02:00

Add inactivity-based session timeout (closes #66) #105

clawbot referenced this pull request 2026-08-09 08:06:26 +02:00

Add inactivity-based session timeout (closes #66) #105

clawbot referenced this pull request 2026-08-09 08:18:55 +02:00

Add inactivity-based session timeout (closes #66) #105

clawbot referenced this pull request 2026-08-10 14:30:12 +02:00

TOP PRIORITY: consolidate all open PRs onto one next branch, one PR #110

clawbot marked the pull request as work in progress 2026-08-10 14:37:30 +02:00

clawbot added needs-rebase and removed merge-ready labels 2026-08-10 14:40:51 +02:00

sneak was unassigned by clawbot 2026-08-10 14:41:01 +02:00

clawbot self-assigned this 2026-08-10 14:41:01 +02:00

clawbot marked the pull request as ready for review 2026-08-10 15:41:55 +02:00

clawbot changed target branch from main to next 2026-08-10 15:41:56 +02:00

clawbot added 1 commit 2026-08-10 15:41:56 +02:00

Fail loudly on set-but-unparseable env config values (closes #80) ... 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 referenced this pull request 2026-08-10 15:45:30 +02:00

Milestone 1.0.0: internet-facing readiness #111

clawbot force-pushed issue-80-config-fail-loud from 985464dcf9 to 949c032265 2026-08-10 15:59:45 +02:00 Compare

clawbot merged commit 45890d4f82 into next 2026-08-10 16:06:12 +02:00

clawbot deleted branch issue-80-config-fail-loud 2026-08-10 16:06:12 +02:00

clawbot referenced this issue from a commit 2026-08-10 16:06:13 +02:00

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

clawbot referenced this pull request 2026-08-11 14:23:29 +02:00

Rate-limit the public webhook receiver endpoint (closes #64) #87

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

merge-ready

needs-checks

needs-rebase

needs-review

needs-rework

No labels needs-rebase

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

clawbot sneak (Jeffrey Paul)

No Assignees clawbot

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#92