README.md:110-119 presents these as "Key settings" and says "See config.example.yml for all options with defaults":
access_control_allow_origin — CORS origin
upstream_fetch_timeout — timeout for origin requests
upstream_max_response_size — max origin response size
downstream_timeout — client response timeout
None of the four exist. isKnownConfigKey (internal/config/config.go:217-227) permits only: debug, maintenance_mode, port, state_dir, sentry_dsn, db_url, metrics, signing_key, allowlist_hosts, allow_http, upstream_connections_per_host, env.
Since #52/#53 landed strict unknown-key rejection, a user who follows the README does not get a silently-ignored key — the daemon refuses to start with unknown config keys: .... That is the correct behavior for an unknown key (repo policy: set-but-unparseable config must abort), but it means our own documentation is a startup-breaking trap.
The underlying values all exist, hardcoded:
CORS origin: []string{"*"} at internal/middleware/middleware.go:114
Fetch timeout / max response size: DefaultFetchTimeout / DefaultMaxResponseSize at internal/httpfetcher/httpfetcher.go:22-23; internal/handlers/handlers.go:76-80 currently overrides only AllowHTTP and MaxConnectionsPerHost
Downstream timeout: HTTPWriteTimeout at internal/server/http.go:12
access_control_allow_origin defaulting to * is itself worth a look while implementing — a wide-open CORS default on an image proxy is defensible but should be a deliberate, documented choice rather than an accident.
Definition of done
All four keys implemented: added to the Config struct, to isKnownConfigKey, parsed with the strict getters (a set-but-invalid value must abort startup naming the key and the offending value, per repo policy — no silent fallback), and plumbed to the code that currently hardcodes each value.
Durations and byte sizes parse consistently with how the rest of the config handles those types; if no precedent exists, pick one and document it in config.example.yml.
Each key is listed in config.example.yml with its default and a comment.
README's config section stays accurate (and the "all options with defaults" claim becomes true — see the separate docs issue for the remaining undocumented keys).
Failing tests first: for each key, a test that a valid value takes effect and an invalid value aborts startup naming key and value, following the table-driven pattern in internal/config/config_validation_internal_test.go.
make check green.
Scope note
If this is more comfortable as four commits, split by key — but keep it one PR, since the shared work is the strict-getter plumbing.
Verified against `main` at `61f42e6`.
`README.md:110-119` presents these as "Key settings" and says "See `config.example.yml` for all options with defaults":
- `access_control_allow_origin` — CORS origin
- `upstream_fetch_timeout` — timeout for origin requests
- `upstream_max_response_size` — max origin response size
- `downstream_timeout` — client response timeout
None of the four exist. `isKnownConfigKey` (`internal/config/config.go:217-227`) permits only: `debug`, `maintenance_mode`, `port`, `state_dir`, `sentry_dsn`, `db_url`, `metrics`, `signing_key`, `allowlist_hosts`, `allow_http`, `upstream_connections_per_host`, `env`.
Since #52/#53 landed strict unknown-key rejection, a user who follows the README does not get a silently-ignored key — **the daemon refuses to start** with `unknown config keys: ...`. That is the correct behavior for an unknown key (repo policy: set-but-unparseable config must abort), but it means our own documentation is a startup-breaking trap.
The underlying values all exist, hardcoded:
- CORS origin: `[]string{"*"}` at `internal/middleware/middleware.go:114`
- Fetch timeout / max response size: `DefaultFetchTimeout` / `DefaultMaxResponseSize` at `internal/httpfetcher/httpfetcher.go:22-23`; `internal/handlers/handlers.go:76-80` currently overrides only `AllowHTTP` and `MaxConnectionsPerHost`
- Downstream timeout: `HTTPWriteTimeout` at `internal/server/http.go:12`
`access_control_allow_origin` defaulting to `*` is itself worth a look while implementing — a wide-open CORS default on an image proxy is defensible but should be a deliberate, documented choice rather than an accident.
## Definition of done
1. All four keys implemented: added to the `Config` struct, to `isKnownConfigKey`, parsed with the **strict** getters (a set-but-invalid value must abort startup naming the key and the offending value, per repo policy — no silent fallback), and plumbed to the code that currently hardcodes each value.
2. Durations and byte sizes parse consistently with how the rest of the config handles those types; if no precedent exists, pick one and document it in `config.example.yml`.
3. Each key is listed in `config.example.yml` with its default and a comment.
4. README's config section stays accurate (and the "all options with defaults" claim becomes true — see the separate docs issue for the remaining undocumented keys).
5. Failing tests first: for each key, a test that a valid value takes effect and an invalid value aborts startup naming key and value, following the table-driven pattern in `internal/config/config_validation_internal_test.go`.
6. `make check` green.
## Scope note
If this is more comfortable as four commits, split by key — but keep it one PR, since the shared work is the strict-getter plumbing.
clawbot
added this to the 1.0.0 milestone 2026-08-09 03:43:07 +02:00
Plan, checked against current next (the four keys are still missing; the values are still fixed in the same places, at different line numbers):
Add access_control_allow_origin (default *), upstream_fetch_timeout (default 30s), upstream_max_response_size (default 52428800, 50 MiB) and downstream_timeout (default 60s) to the config, to the list that pairs each key with its environment variable (PIXA_ plus the key in upper case), to config.example.yml and to the README table.
Durations are Go duration strings (30s, 2m) and must be positive; a bare number such as 30 is refused, since it has no unit. The size is a whole number of bytes, read like cache_max_bytes, and must be positive. config.example.yml says both once.
access_control_allow_origin is * or one origin written as scheme and host, such as https://example.com; anything else (no scheme, a path or trailing slash, empty) aborts startup naming the key and value. It goes to the CORS middleware; where CORS applies is unchanged (that is #98).
upstream_fetch_timeout and upstream_max_response_size go to the upstream fetcher; the size also stays the image decoder's input limit, as today.
downstream_timeout replaces the fixed 60s for both the server's write timeout and the per-request timeout.
Tests first, in their own commit: defaults, valid values, and invalid values aborting startup, for each key, from the file and from the environment.
Model: opus-5-5
Plan, checked against current `next` (the four keys are still missing; the values are still fixed in the same places, at different line numbers):
- Add `access_control_allow_origin` (default `*`), `upstream_fetch_timeout` (default `30s`), `upstream_max_response_size` (default `52428800`, 50 MiB) and `downstream_timeout` (default `60s`) to the config, to the list that pairs each key with its environment variable (`PIXA_` plus the key in upper case), to `config.example.yml` and to the README table.
- Durations are Go duration strings (`30s`, `2m`) and must be positive; a bare number such as `30` is refused, since it has no unit. The size is a whole number of bytes, read like `cache_max_bytes`, and must be positive. `config.example.yml` says both once.
- `access_control_allow_origin` is `*` or one origin written as scheme and host, such as `https://example.com`; anything else (no scheme, a path or trailing slash, empty) aborts startup naming the key and value. It goes to the CORS middleware; where CORS applies is unchanged (that is https://git.eeqj.de/sneak/pixa/issues/98).
- `upstream_fetch_timeout` and `upstream_max_response_size` go to the upstream fetcher; the size also stays the image decoder's input limit, as today.
- `downstream_timeout` replaces the fixed 60s for both the server's write timeout and the per-request timeout.
- Tests first, in their own commit: defaults, valid values, and invalid values aborting startup, for each key, from the file and from the environment.
Model: opus-5-5
Question for sneak: approve two edits to existing tests in #142? This repo's rules require your explicit approval before an existing test is changed.
Neither edit weakens a check; both extend it to the four new settings:
TestEnvironmentSetsEveryKey sets every PIXA_ variable and compares the whole resulting config. Adding four settings adds four variables and four expected fields; without them it fails, because the comparison covers every field.
TestNewHTTPServerTimeouts compared the server's write timeout to the fixed HTTPWriteTimeout. That value is now the downstream_timeout setting, so the test gives the setting a value and compares against it.
Options: (1) approve both edits (recommended, since no assertion is dropped or loosened); (2) refuse. Then no setting can be added at all, because the first test compares every field of the config; the README's four settings would have to be removed from the README instead.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
Question for sneak: approve two edits to existing tests in https://git.eeqj.de/sneak/pixa/pulls/142? This repo's rules require your explicit approval before an existing test is changed.
Neither edit weakens a check; both extend it to the four new settings:
- `TestEnvironmentSetsEveryKey` sets every `PIXA_` variable and compares the whole resulting config. Adding four settings adds four variables and four expected fields; without them it fails, because the comparison covers every field.
- `TestNewHTTPServerTimeouts` compared the server's write timeout to the fixed `HTTPWriteTimeout`. That value is now the `downstream_timeout` setting, so the test gives the setting a value and compares against it.
Options: (1) approve both edits (recommended, since no assertion is dropped or loosened); (2) refuse. Then no setting can be added at all, because the first test compares every field of the config; the README's four settings would have to be removed from the README instead.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
sneak
was assigned by clawbot2026-09-28 20:22:31 +02:00
One more approval needed for #142, again an existing test.
After rebasing onto next, the review found that the login rate limit tests (newTestServer in internal/server/login_rate_limit_internal_test.go, from #143) build a config with no downstream_timeout. Now that the per-request timeout comes from that setting, those tests run every request with a zero timeout. They still pass, but no longer against the routes pixa serves. The fix is one line in that helper: set DownstreamTimeout to its default, config.DefaultDownstreamTimeout. No assertion changes.
Options: (1) approve the one-line helper edit (recommended); (2) refuse, and the rate limit tests keep running with a zero request timeout.
Model: opus-5-5
One more approval needed for https://git.eeqj.de/sneak/pixa/pulls/142, again an existing test.
After rebasing onto `next`, the review found that the login rate limit tests (`newTestServer` in `internal/server/login_rate_limit_internal_test.go`, from https://git.eeqj.de/sneak/pixa/pulls/143) build a config with no `downstream_timeout`. Now that the per-request timeout comes from that setting, those tests run every request with a zero timeout. They still pass, but no longer against the routes pixa serves. The fix is one line in that helper: set `DownstreamTimeout` to its default, `config.DefaultDownstreamTimeout`. No assertion changes.
Options: (1) approve the one-line helper edit (recommended); (2) refuse, and the rate limit tests keep running with a zero request timeout.
Model: opus-5-5
sneak
was assigned by clawbot2026-09-29 02:36:41 +02:00
sneak
was unassigned by clawbot2026-09-29 02:49:24 +02:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Verified against
mainat61f42e6.README.md:110-119presents these as "Key settings" and says "Seeconfig.example.ymlfor all options with defaults":access_control_allow_origin— CORS originupstream_fetch_timeout— timeout for origin requestsupstream_max_response_size— max origin response sizedownstream_timeout— client response timeoutNone of the four exist.
isKnownConfigKey(internal/config/config.go:217-227) permits only:debug,maintenance_mode,port,state_dir,sentry_dsn,db_url,metrics,signing_key,allowlist_hosts,allow_http,upstream_connections_per_host,env.Since #52/#53 landed strict unknown-key rejection, a user who follows the README does not get a silently-ignored key — the daemon refuses to start with
unknown config keys: .... That is the correct behavior for an unknown key (repo policy: set-but-unparseable config must abort), but it means our own documentation is a startup-breaking trap.The underlying values all exist, hardcoded:
[]string{"*"}atinternal/middleware/middleware.go:114DefaultFetchTimeout/DefaultMaxResponseSizeatinternal/httpfetcher/httpfetcher.go:22-23;internal/handlers/handlers.go:76-80currently overrides onlyAllowHTTPandMaxConnectionsPerHostHTTPWriteTimeoutatinternal/server/http.go:12access_control_allow_origindefaulting to*is itself worth a look while implementing — a wide-open CORS default on an image proxy is defensible but should be a deliberate, documented choice rather than an accident.Definition of done
Configstruct, toisKnownConfigKey, parsed with the strict getters (a set-but-invalid value must abort startup naming the key and the offending value, per repo policy — no silent fallback), and plumbed to the code that currently hardcodes each value.config.example.yml.config.example.ymlwith its default and a comment.internal/config/config_validation_internal_test.go.make checkgreen.Scope note
If this is more comfortable as four commits, split by key — but keep it one PR, since the shared work is the strict-getter plumbing.
Plan, checked against current
next(the four keys are still missing; the values are still fixed in the same places, at different line numbers):access_control_allow_origin(default*),upstream_fetch_timeout(default30s),upstream_max_response_size(default52428800, 50 MiB) anddownstream_timeout(default60s) to the config, to the list that pairs each key with its environment variable (PIXA_plus the key in upper case), toconfig.example.ymland to the README table.30s,2m) and must be positive; a bare number such as30is refused, since it has no unit. The size is a whole number of bytes, read likecache_max_bytes, and must be positive.config.example.ymlsays both once.access_control_allow_originis*or one origin written as scheme and host, such ashttps://example.com; anything else (no scheme, a path or trailing slash, empty) aborts startup naming the key and value. It goes to the CORS middleware; where CORS applies is unchanged (that is #98).upstream_fetch_timeoutandupstream_max_response_sizego to the upstream fetcher; the size also stays the image decoder's input limit, as today.downstream_timeoutreplaces the fixed 60s for both the server's write timeout and the per-request timeout.Model: opus-5-5
Built as planned above, in #142.
Model: opus-5-5
Question for sneak: approve two edits to existing tests in #142? This repo's rules require your explicit approval before an existing test is changed.
Neither edit weakens a check; both extend it to the four new settings:
TestEnvironmentSetsEveryKeysets everyPIXA_variable and compares the whole resulting config. Adding four settings adds four variables and four expected fields; without them it fails, because the comparison covers every field.TestNewHTTPServerTimeoutscompared the server's write timeout to the fixedHTTPWriteTimeout. That value is now thedownstream_timeoutsetting, so the test gives the setting a value and compares against it.Options: (1) approve both edits (recommended, since no assertion is dropped or loosened); (2) refuse. Then no setting can be added at all, because the first test compares every field of the config; the README's four settings would have to be removed from the README instead.
Until you answer, the PR goes through review but is not merged; other work carries on.
Model: opus-5-5
1 edits approved
One more approval needed for #142, again an existing test.
After rebasing onto
next, the review found that the login rate limit tests (newTestServerininternal/server/login_rate_limit_internal_test.go, from #143) build a config with nodownstream_timeout. Now that the per-request timeout comes from that setting, those tests run every request with a zero timeout. They still pass, but no longer against the routes pixa serves. The fix is one line in that helper: setDownstreamTimeoutto its default,config.DefaultDownstreamTimeout. No assertion changes.Options: (1) approve the one-line helper edit (recommended); (2) refuse, and the rate limit tests keep running with a zero request timeout.
Model: opus-5-5
1 approved
clawbot referenced this issue2026-09-29 08:49:57 +02:00
clawbot referenced this issue2026-09-29 13:01:47 +02:00
clawbot referenced this issue2026-10-13 00:20:04 +02:00