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

P0: validate configuration on startup, fail fast on bad config #52

New Issue
Closed
opened 2026-08-07 18:06:54 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-07 18:06:54 +02:00
Collaborator
Copy Link

pixad currently starts with whatever the config file contains; bad values surface later as runtime errors. Production P0 from TODO.md Future Steps (queued after #51): validate the full configuration at startup and exit nonzero with a clear error before binding the listener.

Definition of done

  1. Every config option is validated at load time: required values present (signing_key unless a documented keyless mode is intended — decide and document), values in range (ports, timeouts, sizes positive and sane), allowlist_hosts entries well-formed, paths creatable/writable, etc.
  2. Unknown keys in the YAML file are an error, not silently ignored (catches typos like the old source_host_whitelist/whitelist_hosts README mismatch).
  3. Per the repo's no-silent-fallback rule: an INVALID explicit value is always an error; defaults apply only to OMITTED values.
  4. Error messages name the offending key and value.
  5. TDD: failing tests first covering representative invalid configs and the unknown-key case; then the implementation.
  6. make check green; TODO.md Workflow bookkeeping in the finishing commit; finishing commit title ends with (closes #N) for this issue.

Process

Standard workflow: feature branch from main, direction via comments on this issue, discussion on the PR, adversarial review, merge-ready + assign sneak on pass.

`pixad` currently starts with whatever the config file contains; bad values surface later as runtime errors. Production P0 from `TODO.md` Future Steps (queued after #51): validate the full configuration at startup and exit nonzero with a clear error before binding the listener. ## Definition of done 1. Every config option is validated at load time: required values present (`signing_key` unless a documented keyless mode is intended — decide and document), values in range (ports, timeouts, sizes positive and sane), `allowlist_hosts` entries well-formed, paths creatable/writable, etc. 2. Unknown keys in the YAML file are an error, not silently ignored (catches typos like the old `source_host_whitelist`/`whitelist_hosts` README mismatch). 3. Per the repo's no-silent-fallback rule: an INVALID explicit value is always an error; defaults apply only to OMITTED values. 4. Error messages name the offending key and value. 5. TDD: failing tests first covering representative invalid configs and the unknown-key case; then the implementation. 6. `make check` green; `TODO.md` Workflow bookkeeping in the finishing commit; finishing commit title ends with ` (closes #N)` for this issue. ## Process Standard workflow: feature branch from `main`, direction via comments on this issue, discussion on the PR, adversarial review, `merge-ready` + assign sneak on pass.
clawbot commented 2026-08-07 18:25:46 +02:00
Author
Collaborator
Copy Link

Picking this up. Definition of done and implementation plan follow; PR to come on a feature branch from current main.

Definition of done

  1. A config value that is SET but unparseable or invalid is a startup-aborting error. Defaults apply ONLY to OMITTED keys. This is the core no-silent-fallback rule and it currently does not hold: the helpers in internal/config/config.go (getString, getInt, getBool) swallow every conversion error and silently return the default, and loadConfigFile logs a warning and CONTINUES when a config file at a standard location exists but fails to parse. Both become hard errors.
  2. Unknown keys at the top level of the config file are a startup-aborting error naming each unknown key (catches typos like the old whitelist_hosts vs allowlist_hosts mismatch). The env section stays permitted (smartconfig consumes it for environment injection), and nested metrics subkeys are validated too (username/password only).
  3. Range/sanity validation on every option: port an integer in 1-65535 (no float truncation: 8080.5 is an error, not 8080); upstream_connections_per_host an integer of at least 1; signing_key required and at least 32 characters (existing rule, kept — no keyless mode; documented in the config error message); allowlist_hosts entries must be bare hostnames (non-empty strings, no scheme, no slash, no whitespace); state_dir non-empty, must be creatable and writable (verified at startup with a probe file); sentry_dsn if set must parse as a URL with scheme and host; metrics.username and metrics.password must be set together or not at all; debug/maintenance_mode/allow_http must be booleans or ParseBool-able strings, not arbitrary numbers.
  4. Every error message names the offending key and the offending value.
  5. Startup path: config.New returns the error, fx aborts before binding the listener, process exits nonzero.

Implementation plan

  • internal/config/config.go: extract a newFromSmartConfig(sc) construction/validation function that New wraps (keeps fx wiring untouched, makes the logic testable without fx). Replace the silent-default getters with strict variants that distinguish key-absent (default) from key-present-but-invalid (error). Add validateKnownKeys against the canonical key set (debug, maintenance_mode, port, state_dir, sentry_dsn, db_url, metrics, signing_key, allowlist_hosts, allow_http, upstream_connections_per_host, env). Extend validate() with the range/wellformedness rules above. Make a parse failure of an existing config file fatal in loadConfigFile instead of warn-and-continue.
  • internal/config/config_test.go: TDD — failing tests committed first, covering (a) omitted keys use defaults (minimal config with only a valid signing_key), (b) a table of set-but-invalid values each asserting startup fails with an error naming the key (invalid port string, port 0, port 70000, float port, non-bool debug, zero/negative upstream_connections_per_host, allowlist entry with scheme, non-string allowlist entry, short signing_key, missing signing_key, metrics.username without password, invalid sentry_dsn, explicitly empty state_dir), (c) unknown top-level key aborts naming the key, (d) env section is not flagged as unknown.
  • Workflow: feature branch from main, tests committed first, then the implementation, make fmt on touched markdown, make check green, TODO.md bookkeeping in the finishing commit, PR title ending with (closes #52), label needs-review.
Picking this up. Definition of done and implementation plan follow; PR to come on a feature branch from current `main`. ## Definition of done 1. A config value that is SET but unparseable or invalid is a startup-aborting error. Defaults apply ONLY to OMITTED keys. This is the core no-silent-fallback rule and it currently does not hold: the helpers in `internal/config/config.go` (`getString`, `getInt`, `getBool`) swallow every conversion error and silently return the default, and `loadConfigFile` logs a warning and CONTINUES when a config file at a standard location exists but fails to parse. Both become hard errors. 2. Unknown keys at the top level of the config file are a startup-aborting error naming each unknown key (catches typos like the old `whitelist_hosts` vs `allowlist_hosts` mismatch). The `env` section stays permitted (smartconfig consumes it for environment injection), and nested `metrics` subkeys are validated too (`username`/`password` only). 3. Range/sanity validation on every option: `port` an integer in 1-65535 (no float truncation: `8080.5` is an error, not 8080); `upstream_connections_per_host` an integer of at least 1; `signing_key` required and at least 32 characters (existing rule, kept — no keyless mode; documented in the config error message); `allowlist_hosts` entries must be bare hostnames (non-empty strings, no scheme, no slash, no whitespace); `state_dir` non-empty, must be creatable and writable (verified at startup with a probe file); `sentry_dsn` if set must parse as a URL with scheme and host; `metrics.username` and `metrics.password` must be set together or not at all; `debug`/`maintenance_mode`/`allow_http` must be booleans or ParseBool-able strings, not arbitrary numbers. 4. Every error message names the offending key and the offending value. 5. Startup path: `config.New` returns the error, fx aborts before binding the listener, process exits nonzero. ## Implementation plan - `internal/config/config.go`: extract a `newFromSmartConfig(sc)` construction/validation function that `New` wraps (keeps fx wiring untouched, makes the logic testable without fx). Replace the silent-default getters with strict variants that distinguish key-absent (default) from key-present-but-invalid (error). Add `validateKnownKeys` against the canonical key set (`debug`, `maintenance_mode`, `port`, `state_dir`, `sentry_dsn`, `db_url`, `metrics`, `signing_key`, `allowlist_hosts`, `allow_http`, `upstream_connections_per_host`, `env`). Extend `validate()` with the range/wellformedness rules above. Make a parse failure of an existing config file fatal in `loadConfigFile` instead of warn-and-continue. - `internal/config/config_test.go`: TDD — failing tests committed first, covering (a) omitted keys use defaults (minimal config with only a valid `signing_key`), (b) a table of set-but-invalid values each asserting startup fails with an error naming the key (invalid port string, port 0, port 70000, float port, non-bool `debug`, zero/negative `upstream_connections_per_host`, allowlist entry with scheme, non-string allowlist entry, short `signing_key`, missing `signing_key`, `metrics.username` without password, invalid `sentry_dsn`, explicitly empty `state_dir`), (c) unknown top-level key aborts naming the key, (d) `env` section is not flagged as unknown. - Workflow: feature branch from `main`, tests committed first, then the implementation, `make fmt` on touched markdown, `make check` green, `TODO.md` bookkeeping in the finishing commit, PR title ending with (closes #52), label `needs-review`.
clawbot commented 2026-08-07 18:38:05 +02:00
Author
Collaborator
Copy Link

Implementation is up as PR #53 (branch feature/config-validation, labeled needs-review), executed per the plan above: TDD (failing enforcement tests committed first, then the implementation), make check green, end-to-end verified (pixad exits 1 with config key "port": value "banana" is not an integer on a bad value and unknown config keys: whitelist_hosts on a typo'd key). One deviation from the plan, documented in the PR: the lenient getStringSlice helper is retained because existing tests exercise it and test modification needs explicit approval — strictness for allowlist_hosts is enforced on the raw value by validateAllowlistHostsValue before extraction, so the no-silent-fallback guarantee holds regardless. Awaiting adversarial review per the standard workflow.

Implementation is up as PR #53 (branch `feature/config-validation`, labeled `needs-review`), executed per the plan above: TDD (failing enforcement tests committed first, then the implementation), `make check` green, end-to-end verified (`pixad` exits 1 with `config key "port": value "banana" is not an integer` on a bad value and `unknown config keys: whitelist_hosts` on a typo'd key). One deviation from the plan, documented in the PR: the lenient `getStringSlice` helper is retained because existing tests exercise it and test modification needs explicit approval — strictness for `allowlist_hosts` is enforced on the raw value by `validateAllowlistHostsValue` before extraction, so the no-silent-fallback guarantee holds regardless. Awaiting adversarial review per the standard workflow.
sneak closed this issue 2026-08-07 22:39:40 +02:00
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/pixa#52
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?