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

Config parsing should fail loudly on set-but-unparseable env values (envInt, etc.) #80

New Issue
Closed
opened 2026-08-07 15:29:22 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-07 15:29:22 +02:00
Collaborator
Copy Link

Extends the fix on PR #78 (#63).

The config env helpers (envInt, and any peers) silently fall back to the default when an env var is set but unparseable — the same silent-failure pattern @sneak flagged for the duration parser: a set-but-invalid config value should fail loudly and prevent startup, not be silently ignored.

Definition of done:

  • envInt (and any other set-but-unparseable-swallowing helpers) return an error on a set-but-unparseable value
  • config.New propagates it so startup aborts on bad config
  • an UNSET value still legitimately uses the default
  • covered by tests

Note: 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.

Extends the fix on PR #78 (#63). The config env helpers (`envInt`, and any peers) silently fall back to the default when an env var is set but unparseable — the same silent-failure pattern @sneak flagged for the duration parser: a set-but-invalid config value should fail loudly and prevent startup, not be silently ignored. Definition of done: - `envInt` (and any other set-but-unparseable-swallowing helpers) return an error on a set-but-unparseable value - `config.New` propagates it so startup aborts on bad config - an UNSET value still legitimately uses the default - covered by tests Note: 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.
clawbot commented 2026-08-09 03:34:31 +02:00
Author
Collaborator
Copy Link

Implementation requirements

Baseline: main @ 4f5ecb1. All work in internal/config unless the audit in step 5 finds offenders elsewhere.

Policy being encoded

A set-but-invalid environment value aborts startup. Defaults apply only to keys that are unset (or empty). There is no third behaviour — never silently substitute a default for a value the operator actually set.

1. Add envPositiveInt

Add it verbatim as PR #87 defines it on issue-64-receiver-rate-limit — same function name, same signature envPositiveInt(key string, defaultValue int) (int, error), same package-level ErrNonPositiveValue, same error strings:

  • unset/empty -> defaultValue, nil
  • unparseable -> 0, fmt.Errorf("invalid integer for %s: %q: %w", key, v, err)
  • parses to less than 1 -> 0, fmt.Errorf("%w: %s must be at least 1, got %q", ErrNonPositiveValue, key, v)

Copying it byte-for-byte is deliberate: #87 is already merge-ready and will very likely land first, so an identical definition makes the rebase a trivial delete-one-copy instead of a semantic merge.

2. envInt must stop swallowing

envInt currently returns defaultValue when the value is set but strconv.Atoi fails. Its only caller is PORT. Fix by removing envInt entirely and parsing PORT through envPositiveInt, plus an explicit upper-bound check: a TCP port must be in 1..65535. Add a package-level ErrInvalidPort for the out-of-range case with an error naming the key and the bad value. If you keep envInt for any reason, it must return (int, error) — a silent-fallback variant must not survive anywhere in the package.

3. envBool must stop swallowing

envBool currently treats every non-empty value that is not true/1 as false, so DEBUG=yes, DEBUG=on, and a typo like DEBUG=ture all silently disable debug. Change it to envBool(key string, defaultValue bool) (bool, error):

  • unset/empty -> defaultValue, nil
  • otherwise parse with strconv.ParseBool (stdlib, per our library policy — do not hand-roll a spelling table)
  • parse failure -> a wrapped error naming the key and the bad value

Callers: DEBUG, MAINTENANCE_MODE. Yes, this narrows the accepted set (mixed-case oddities like TrUe that EqualFold used to accept now fail) — that is the intended fail-loud behaviour, and strconv.ParseBool accepts 1 t T TRUE true True 0 f F FALSE false False.

4. envString and envDuration

envString parses nothing and needs no change; envDuration is already fail-loud (#78). Leave both alone, but say explicitly in the PR body that they were audited, so the reviewer does not have to re-derive it.

5. Audit beyond internal/config

Grep the whole repo for os.Getenv / LookupEnv and check every parse site for the same silent-fallback pattern. Fix any you find under this issue, or state in the PR body that there are none outside internal/config. Do not leave the audit undone.

6. Propagation

config.New returns the first error it hits so fx aborts startup. Errors must name the offending key and the offending value.

7. Tests

Table-driven, one table per helper, three classes each: unset -> default, valid -> parsed, set-but-invalid -> error. Plus at least one config.New-level test proving a bad PORT and a bad DEBUG each abort with an error rather than returning a config. Use t.Setenv so nothing leaks between tests.

8. Docs

  • README.md: state in the configuration section that any environment variable that is set but unparseable aborts startup, and that defaults apply only to unset variables. List the accepted boolean spellings.
  • TODO.md: update in the same commit as the code.

Definition of done

  • No helper in internal/config silently falls back to a default for a set-but-invalid value.
  • An unset variable still gets its default.
  • make check green; validated via the repo's own entrypoints only (make check / script/cibuild) — never raw go/golangci-lint.
  • .golangci.yml untouched (org-standard, pinned on main via #86).
  • Single commit, title ending in (closes #80).
  • No attribution trailers.
## Implementation requirements Baseline: `main` @ `4f5ecb1`. All work in `internal/config` unless the audit in step 5 finds offenders elsewhere. ### Policy being encoded A set-but-invalid environment value **aborts startup**. Defaults apply **only** to keys that are unset (or empty). There is no third behaviour — never silently substitute a default for a value the operator actually set. ### 1. Add `envPositiveInt` Add it **verbatim** as PR #87 defines it on `issue-64-receiver-rate-limit` — same function name, same signature `envPositiveInt(key string, defaultValue int) (int, error)`, same package-level `ErrNonPositiveValue`, same error strings: - unset/empty -> `defaultValue, nil` - unparseable -> `0, fmt.Errorf("invalid integer for %s: %q: %w", key, v, err)` - parses to less than 1 -> `0, fmt.Errorf("%w: %s must be at least 1, got %q", ErrNonPositiveValue, key, v)` Copying it byte-for-byte is deliberate: #87 is already merge-ready and will very likely land first, so an identical definition makes the rebase a trivial delete-one-copy instead of a semantic merge. ### 2. `envInt` must stop swallowing `envInt` currently returns `defaultValue` when the value is set but `strconv.Atoi` fails. Its only caller is `PORT`. Fix by removing `envInt` entirely and parsing `PORT` through `envPositiveInt`, plus an explicit upper-bound check: a TCP port must be in `1..65535`. Add a package-level `ErrInvalidPort` for the out-of-range case with an error naming the key and the bad value. If you keep `envInt` for any reason, it must return `(int, error)` — a silent-fallback variant must not survive anywhere in the package. ### 3. `envBool` must stop swallowing `envBool` currently treats every non-empty value that is not `true`/`1` as `false`, so `DEBUG=yes`, `DEBUG=on`, and a typo like `DEBUG=ture` all silently disable debug. Change it to `envBool(key string, defaultValue bool) (bool, error)`: - unset/empty -> `defaultValue, nil` - otherwise parse with `strconv.ParseBool` (stdlib, per our library policy — do not hand-roll a spelling table) - parse failure -> a wrapped error naming the key and the bad value Callers: `DEBUG`, `MAINTENANCE_MODE`. Yes, this narrows the accepted set (mixed-case oddities like `TrUe` that `EqualFold` used to accept now fail) — that is the intended fail-loud behaviour, and `strconv.ParseBool` accepts `1 t T TRUE true True 0 f F FALSE false False`. ### 4. `envString` and `envDuration` `envString` parses nothing and needs no change; `envDuration` is already fail-loud (#78). Leave both alone, but say explicitly in the PR body that they were audited, so the reviewer does not have to re-derive it. ### 5. Audit beyond `internal/config` Grep the whole repo for `os.Getenv` / `LookupEnv` and check every parse site for the same silent-fallback pattern. Fix any you find under this issue, or state in the PR body that there are none outside `internal/config`. Do not leave the audit undone. ### 6. Propagation `config.New` returns the first error it hits so fx aborts startup. Errors must name the offending key and the offending value. ### 7. Tests Table-driven, one table per helper, three classes each: unset -> default, valid -> parsed, set-but-invalid -> error. Plus at least one `config.New`-level test proving a bad `PORT` and a bad `DEBUG` each abort with an error rather than returning a config. Use `t.Setenv` so nothing leaks between tests. ### 8. Docs - `README.md`: state in the configuration section that any environment variable that is set but unparseable aborts startup, and that defaults apply only to unset variables. List the accepted boolean spellings. - `TODO.md`: update in the **same commit** as the code. ### Definition of done - No helper in `internal/config` silently falls back to a default for a set-but-invalid value. - An unset variable still gets its default. - `make check` green; validated via the repo's own entrypoints only (`make check` / `script/cibuild`) — never raw `go`/`golangci-lint`. - `.golangci.yml` untouched (org-standard, pinned on `main` via #86). - Single commit, title ending in ` (closes #80)`. - No attribution trailers.
clawbot commented 2026-08-09 03:36:43 +02:00
Author
Collaborator
Copy Link

Implementation plan

Branch issue-80-config-fail-loud off main @ 4f5ecb1, single commit
titled with (closes #80).

  1. internal/config/config.go: add envPositiveInt(key string, defaultValue int) (int, error) byte-for-byte as PR #87 defines it,
    with the same package-level ErrNonPositiveValue and the same error
    strings, so a rebase after #87 lands is a delete-one-copy operation.
  2. Delete envInt entirely. PORT is parsed through a new envPort
    wrapper: envPositiveInt for the unset/unparseable/<1 cases plus an
    explicit upper bound (maxPort = 65535) returning a new package-level
    ErrInvalidPort naming the key and the bad value.
  3. Rewrite envBool as envBool(key string, defaultValue bool) (bool, error) using strconv.ParseBool; unset/empty keeps the default,
    anything else that fails to parse is a wrapped error naming the key
    and value. Callers DEBUG and MAINTENANCE_MODE updated.
  4. envString and envDuration audited and left alone (envString
    parses nothing; envDuration was already made fail-loud in #78) —
    stated explicitly in the PR body.
  5. Repo-wide audit of os.Getenv/os.LookupEnv across tracked files;
    result recorded in the PR body.
  6. config.New returns the first error, so fx aborts startup. If the
    added error handling pushes New past the funlen threshold, the
    WEBHOOKER_ENVIRONMENT block is extracted into resolveEnvironment,
    again copied verbatim from #87 for the same rebase reason.
  7. Tests: table-driven per-helper tables for envPositiveInt, envPort
    and envBool (unset -> default, valid -> parsed, set-but-invalid ->
    error) reaching the unexported helpers through an export_test.go
    shim, plus config.New-level tests proving a bad PORT and a bad
    DEBUG each abort with an error instead of returning a config. All
    env manipulation via t.Setenv.
  8. Docs: README.md configuration section gains the fail-loud rule
    (defaults apply only to unset variables) and the list of accepted
    boolean spellings; TODO.md updated in the same commit; make fmt
    run on the changed markdown.

Verification: make fmt then make check only — no raw go or
golangci-lint invocations. .golangci.yml untouched.

## Implementation plan Branch `issue-80-config-fail-loud` off `main` @ `4f5ecb1`, single commit titled with ` (closes #80)`. 1. `internal/config/config.go`: add `envPositiveInt(key string, defaultValue int) (int, error)` byte-for-byte as PR #87 defines it, with the same package-level `ErrNonPositiveValue` and the same error strings, so a rebase after #87 lands is a delete-one-copy operation. 2. Delete `envInt` entirely. `PORT` is parsed through a new `envPort` wrapper: `envPositiveInt` for the unset/unparseable/&lt;1 cases plus an explicit upper bound (`maxPort = 65535`) returning a new package-level `ErrInvalidPort` naming the key and the bad value. 3. Rewrite `envBool` as `envBool(key string, defaultValue bool) (bool, error)` using `strconv.ParseBool`; unset/empty keeps the default, anything else that fails to parse is a wrapped error naming the key and value. Callers `DEBUG` and `MAINTENANCE_MODE` updated. 4. `envString` and `envDuration` audited and left alone (`envString` parses nothing; `envDuration` was already made fail-loud in #78) — stated explicitly in the PR body. 5. Repo-wide audit of `os.Getenv`/`os.LookupEnv` across tracked files; result recorded in the PR body. 6. `config.New` returns the first error, so fx aborts startup. If the added error handling pushes `New` past the `funlen` threshold, the `WEBHOOKER_ENVIRONMENT` block is extracted into `resolveEnvironment`, again copied verbatim from #87 for the same rebase reason. 7. Tests: table-driven per-helper tables for `envPositiveInt`, `envPort` and `envBool` (unset -&gt; default, valid -&gt; parsed, set-but-invalid -&gt; error) reaching the unexported helpers through an `export_test.go` shim, plus `config.New`-level tests proving a bad `PORT` and a bad `DEBUG` each abort with an error instead of returning a config. All env manipulation via `t.Setenv`. 8. Docs: `README.md` configuration section gains the fail-loud rule (defaults apply only to unset variables) and the list of accepted boolean spellings; `TODO.md` updated in the same commit; `make fmt` run on the changed markdown. Verification: `make fmt` then `make check` only — no raw `go` or `golangci-lint` invocations. `.golangci.yml` untouched.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#80
Delete Branch "%!s()"

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?