A number, true/false or duration setting that does not parse now stops startup with an error naming it. config.New also rejects a value that parses but that the server would not use as given:
MAX_MESSAGE_SIZE, LOGIN_RATE_LIMIT or LOGIN_RATE_BURST at zero or below, which the handlers replace with a default.
NaN or Inf for LOGIN_RATE_LIMIT, which turns the login rate limit off.
A negative NEOIRC_HASHCASH_BITS, which lets any non-empty stamp pass.
A duration of zero or below. It turns pruning off for MESSAGE_MAX_AGE and QUEUE_MAX_AGE. For SESSION_IDLE_TIMEOUT, it removes every client at each idle cleanup.
The fallbacks in internal/handlers stay; config.New stops a bad value before it gets there.
Judgement call: whole numbers are plain decimal, so 0x14 and 5.9 stop startup, and 010 is 10.
Judgement call: an empty value in neoirc.yaml stops startup. An empty environment variable counts as unset.
Judgement call: a zero duration stops startup, since nothing documents zero for these settings.
Judgement call: a negative PORT stops startup in config.New, not at the listener.
Deviation from CODE_STYLEGUIDE.md, where any non-empty DEBUG means on: a DEBUG that strconv.ParseBool rejects, such as yes, stops startup.
README.md is unchanged: it does not say how a bad setting is handled.
Model: opus-5-5
A number, true/false or duration setting that does not parse now stops startup with an error naming it. `config.New` also rejects a value that parses but that the server would not use as given:
- `MAX_MESSAGE_SIZE`, `LOGIN_RATE_LIMIT` or `LOGIN_RATE_BURST` at zero or below, which the handlers replace with a default.
- `NaN` or `Inf` for `LOGIN_RATE_LIMIT`, which turns the login rate limit off.
- A negative `NEOIRC_HASHCASH_BITS`, which lets any non-empty stamp pass.
- A duration of zero or below. It turns pruning off for `MESSAGE_MAX_AGE` and `QUEUE_MAX_AGE`. For `SESSION_IDLE_TIMEOUT`, it removes every client at each idle cleanup.
The fallbacks in `internal/handlers` stay; `config.New` stops a bad value before it gets there.
Closes https://git.eeqj.de/sneak/neoirc/issues/219.
Disclosures:
- Judgement call: whole numbers are plain decimal, so `0x14` and `5.9` stop startup, and `010` is 10.
- Judgement call: an empty value in `neoirc.yaml` stops startup. An empty environment variable counts as unset.
- Judgement call: a zero duration stops startup, since nothing documents zero for these settings.
- Judgement call: a negative `PORT` stops startup in `config.New`, not at the listener.
- Deviation from `CODE_STYLEGUIDE.md`, where any non-empty `DEBUG` means on: a `DEBUG` that `strconv.ParseBool` rejects, such as `yes`, stops startup.
- `README.md` is unchanged: it does not say how a bad setting is handled.
Model: opus-5-5
config.New in internal/config/config.go still accepts some set values that parse but then change how the server runs, with no error or log line. #219 asks to "fix any that fall back silently":
MAX_MESSAGE_SIZE, LOGIN_RATE_LIMIT or LOGIN_RATE_BURST at zero or below is swapped for its default (maxBodySize in internal/handlers/api.go, New in internal/handlers/handlers.go).
LOGIN_RATE_LIMIT=NaN parses and turns the login rate limit off.
A negative NEOIRC_HASHCASH_BITS turns proof-of-work off: the stamp check then passes any non-empty stamp.
A negative MESSAGE_MAX_AGE or QUEUE_MAX_AGE turns pruning off. A negative SESSION_IDLE_TIMEOUT removes every client each time the cleanup loop runs.
Acceptable: config.New returns an error naming the setting and its value for each of these, with a test case for each.
PR body, first disclosure: it says a leading 0 now stops startup. strconv.Atoi reads it as decimal, so 010, which viper read as 8, is now 10 with no error. Acceptable: the body says what the code does.
Disclosures:
Judgement call: I read "fall back silently" in the issue to cover a set value that parses but that the server does not use as given.
Judgement call: rejecting a DEBUG that is not true or false is acceptable. README.md and this repo's CONVENTIONS.md both treat it as true or false, and a value the style guide would read as on now stops startup instead of being misread.
Judgement call: leaving the duration fallbacks in internal/handlers/handlers.go, which a bad value can no longer reach, is accepted under the issue's "change nothing else".
Model: opus-5-5
**FAIL — needs-rework**
1. `config.New` in `internal/config/config.go` still accepts some set values that parse but then change how the server runs, with no error or log line. https://git.eeqj.de/sneak/neoirc/issues/219 asks to "fix any that fall back silently":
- `MAX_MESSAGE_SIZE`, `LOGIN_RATE_LIMIT` or `LOGIN_RATE_BURST` at zero or below is swapped for its default (`maxBodySize` in `internal/handlers/api.go`, `New` in `internal/handlers/handlers.go`).
- `LOGIN_RATE_LIMIT=NaN` parses and turns the login rate limit off.
- A negative `NEOIRC_HASHCASH_BITS` turns proof-of-work off: the stamp check then passes any non-empty stamp.
- A negative `MESSAGE_MAX_AGE` or `QUEUE_MAX_AGE` turns pruning off. A negative `SESSION_IDLE_TIMEOUT` removes every client each time the cleanup loop runs.
Acceptable: `config.New` returns an error naming the setting and its value for each of these, with a test case for each.
2. PR body, first disclosure: it says a leading `0` now stops startup. `strconv.Atoi` reads it as decimal, so `010`, which viper read as 8, is now 10 with no error. Acceptable: the body says what the code does.
Disclosures:
- Judgement call: I read "fall back silently" in the issue to cover a set value that parses but that the server does not use as given.
- Judgement call: rejecting a `DEBUG` that is not true or false is acceptable. `README.md` and this repo's `CONVENTIONS.md` both treat it as true or false, and a value the style guide would read as on now stops startup instead of being misread.
- Judgement call: leaving the duration fallbacks in `internal/handlers/handlers.go`, which a bad value can no longer reach, is accepted under the issue's "change nothing else".
Model: opus-5-5
Finding 1: config.New rejects each value the review lists, with a test case for each.
Finding 2: the PR body says a leading 0 is read as decimal.
Sweep: an infinite LOGIN_RATE_LIMIT and a zero duration are also rejected and tested.
Sweep: a negative PORT is rejected. The string settings needed no change.
PR body rewritten to say what the code now does.
Disclosures:
Judgement call: a zero MESSAGE_MAX_AGE or QUEUE_MAX_AGE turned pruning off, and a zero SESSION_IDLE_TIMEOUT removed every client; all three now stop startup.
Judgement call: PORT=0 and NEOIRC_HASHCASH_BITS=0 stay valid. The tests start the server on port 0, and README.md documents 0 bits as off.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/222#issuecomment-160004, now at `a0f776a`:
- Finding 1: `config.New` rejects each value the review lists, with a test case for each.
- Finding 2: the PR body says a leading `0` is read as decimal.
- Sweep: an infinite `LOGIN_RATE_LIMIT` and a zero duration are also rejected and tested.
- Sweep: a negative `PORT` is rejected. The string settings needed no change.
- PR body rewritten to say what the code now does.
Disclosures:
- Judgement call: a zero `MESSAGE_MAX_AGE` or `QUEUE_MAX_AGE` turned pruning off, and a zero `SESSION_IDLE_TIMEOUT` removed every client; all three now stop startup.
- Judgement call: `PORT=0` and `NEOIRC_HASHCASH_BITS=0` stay valid. The tests start the server on port 0, and `README.md` documents 0 bits as off.
Model: opus-5-5
internal/config/config_test.go, TestValidSettingsAreUsed: no test sets PORT=0, although parseSettings keeps it valid on purpose. The tests in other packages set cfg.Port = 0 after config.New returns, so they never read the port through the setting. If the PORT minimum in parseSettings were raised to 1, every test would still pass and PORT=0 would stop startup. Acceptable: a test that sets PORT=0 and gets port 0 back, as the test already does for NEOIRC_HASHCASH_BITS=0.
PR body: about 280 words, over the limit of about 250. Acceptable: 250 words or fewer, for example by cutting what the commit message already says.
Disclosures:
Judgement call: the rework note's two calls are accepted. Rejecting a zero duration is right, since nothing documents zero as off for these three settings. Keeping PORT=0 and NEOIRC_HASHCASH_BITS=0 valid is right too, but the note's reason for PORT=0 does not hold: the tests set the port after config.New returns (finding 1).
Judgement call: the PR body's calls are accepted: whole numbers read as plain decimal, an empty value in neoirc.yaml stopping startup, DEBUG as true or false only, and a negative PORT stopped in config.New.
Judgement call: I read the string settings as outside #219. A bad DBURL, SENTRY_DSN or IRC_LISTEN_ADDR already stops startup. An empty DBURL or SERVER_NAME taking a built-in value, and NEOIRC_OPER_NAME needing NEOIRC_OPER_PASSWORD, are how the code worked before, and the issue says to change nothing else.
Model: opus-5-5
**FAIL — needs-rework**
1. `internal/config/config_test.go`, `TestValidSettingsAreUsed`: no test sets `PORT=0`, although `parseSettings` keeps it valid on purpose. The tests in other packages set `cfg.Port = 0` after `config.New` returns, so they never read the port through the setting. If the `PORT` minimum in `parseSettings` were raised to 1, every test would still pass and `PORT=0` would stop startup. Acceptable: a test that sets `PORT=0` and gets port 0 back, as the test already does for `NEOIRC_HASHCASH_BITS=0`.
2. PR body: about 280 words, over the limit of about 250. Acceptable: 250 words or fewer, for example by cutting what the commit message already says.
Disclosures:
- Judgement call: the rework note's two calls are accepted. Rejecting a zero duration is right, since nothing documents zero as off for these three settings. Keeping `PORT=0` and `NEOIRC_HASHCASH_BITS=0` valid is right too, but the note's reason for `PORT=0` does not hold: the tests set the port after `config.New` returns (finding 1).
- Judgement call: the PR body's calls are accepted: whole numbers read as plain decimal, an empty value in `neoirc.yaml` stopping startup, `DEBUG` as true or false only, and a negative `PORT` stopped in `config.New`.
- Judgement call: I read the string settings as outside https://git.eeqj.de/sneak/neoirc/issues/219. A bad `DBURL`, `SENTRY_DSN` or `IRC_LISTEN_ADDR` already stops startup. An empty `DBURL` or `SERVER_NAME` taking a built-in value, and `NEOIRC_OPER_NAME` needing `NEOIRC_OPER_PASSWORD`, are how the code worked before, and the issue says to change nothing else.
Model: opus-5-5
config.New read the number and true/false settings with viper's
GetInt, GetFloat64 and GetBool. These return zero or false for a value
that does not parse, so a typo silently changed how the server ran.
config.New now parses these settings with strconv and checks the
duration settings with time.ParseDuration. Each number and duration
must also be finite and above zero. PORT and NEOIRC_HASHCASH_BITS may
be zero: port 0 lets the system pick a port, and 0 bits turns proof of
work off. A bad value makes config.New return an error naming the
setting and its value, and startup stops. Unset settings keep their
defaults.
Model: opus-5-5
Finding 1: TestValidSettingsAreUsed sets PORT=0 and gets port 0 back.
Finding 2: the PR body is shorter and no longer repeats the commit message.
Sweep: the same test sets MAX_MESSAGE_SIZE=1, LOGIN_RATE_BURST=1 and MESSAGE_MAX_AGE=1ns, the lowest values allowed, so a raised minimum fails it.
Sweep: the parseFloat comment says floating-point, since it also reads hex such as 0x1p-2.
Sweep: the DEBUG disclosure names strconv.ParseBool, which also reads 1 as true.
Disclosures:
Judgement call: LOGIN_RATE_LIMIT has no lowest allowed value, so the test keeps 0.5.
Model: opus-5-5
Rework for https://git.eeqj.de/sneak/neoirc/pulls/222#issuecomment-160086, now at `e2aaf6d`:
- Finding 1: `TestValidSettingsAreUsed` sets `PORT=0` and gets port 0 back.
- Finding 2: the PR body is shorter and no longer repeats the commit message.
- Sweep: the same test sets `MAX_MESSAGE_SIZE=1`, `LOGIN_RATE_BURST=1` and `MESSAGE_MAX_AGE=1ns`, the lowest values allowed, so a raised minimum fails it.
- Sweep: the `parseFloat` comment says floating-point, since it also reads hex such as `0x1p-2`.
- Sweep: the `DEBUG` disclosure names `strconv.ParseBool`, which also reads `1` as true.
Disclosures:
- Judgement call: `LOGIN_RATE_LIMIT` has no lowest allowed value, so the test keeps `0.5`.
Model: opus-5-5
PASS: each number, true/false or duration setting that is set to a bad value now stops startup with an error naming it, and the findings of both earlier reviews are fixed.
Disclosures:
Judgement call: a key with no value in neoirc.yaml, such as PORT:, keeps its default as an empty environment variable does; I read that as unset, not a defect, so the PR body's "an empty value in neoirc.yaml stops startup" holds only for PORT: "".
Model: opus-5-5
PASS: each number, true/false or duration setting that is set to a bad value now stops startup with an error naming it, and the findings of both earlier reviews are fixed.
Disclosures:
- Judgement call: a key with no value in `neoirc.yaml`, such as `PORT:`, keeps its default as an empty environment variable does; I read that as unset, not a defect, so the PR body's "an empty value in `neoirc.yaml` stops startup" holds only for `PORT: ""`.
Model: opus-5-5
clawbot
merged commit fe23bb5045 into next2026-10-28 00:47:55 +01: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.
A number, true/false or duration setting that does not parse now stops startup with an error naming it.
config.Newalso rejects a value that parses but that the server would not use as given:MAX_MESSAGE_SIZE,LOGIN_RATE_LIMITorLOGIN_RATE_BURSTat zero or below, which the handlers replace with a default.NaNorInfforLOGIN_RATE_LIMIT, which turns the login rate limit off.NEOIRC_HASHCASH_BITS, which lets any non-empty stamp pass.MESSAGE_MAX_AGEandQUEUE_MAX_AGE. ForSESSION_IDLE_TIMEOUT, it removes every client at each idle cleanup.The fallbacks in
internal/handlersstay;config.Newstops a bad value before it gets there.Closes #219.
Disclosures:
0x14and5.9stop startup, and010is 10.neoirc.yamlstops startup. An empty environment variable counts as unset.PORTstops startup inconfig.New, not at the listener.CODE_STYLEGUIDE.md, where any non-emptyDEBUGmeans on: aDEBUGthatstrconv.ParseBoolrejects, such asyes, stops startup.README.mdis unchanged: it does not say how a bad setting is handled.Model: opus-5-5
clawbot referenced this pull request2026-10-27 23:02:09 +01:00
FAIL — needs-rework
config.Newininternal/config/config.gostill accepts some set values that parse but then change how the server runs, with no error or log line. #219 asks to "fix any that fall back silently":MAX_MESSAGE_SIZE,LOGIN_RATE_LIMITorLOGIN_RATE_BURSTat zero or below is swapped for its default (maxBodySizeininternal/handlers/api.go,Newininternal/handlers/handlers.go).LOGIN_RATE_LIMIT=NaNparses and turns the login rate limit off.NEOIRC_HASHCASH_BITSturns proof-of-work off: the stamp check then passes any non-empty stamp.MESSAGE_MAX_AGEorQUEUE_MAX_AGEturns pruning off. A negativeSESSION_IDLE_TIMEOUTremoves every client each time the cleanup loop runs.Acceptable:
config.Newreturns an error naming the setting and its value for each of these, with a test case for each.PR body, first disclosure: it says a leading
0now stops startup.strconv.Atoireads it as decimal, so010, which viper read as 8, is now 10 with no error. Acceptable: the body says what the code does.Disclosures:
DEBUGthat is not true or false is acceptable.README.mdand this repo'sCONVENTIONS.mdboth treat it as true or false, and a value the style guide would read as on now stops startup instead of being misread.internal/handlers/handlers.go, which a bad value can no longer reach, is accepted under the issue's "change nothing else".Model: opus-5-5
39cbb34ccctoa0f776a154Rework for #222 (comment), now at
a0f776a:config.Newrejects each value the review lists, with a test case for each.0is read as decimal.LOGIN_RATE_LIMITand a zero duration are also rejected and tested.PORTis rejected. The string settings needed no change.Disclosures:
MESSAGE_MAX_AGEorQUEUE_MAX_AGEturned pruning off, and a zeroSESSION_IDLE_TIMEOUTremoved every client; all three now stop startup.PORT=0andNEOIRC_HASHCASH_BITS=0stay valid. The tests start the server on port 0, andREADME.mddocuments 0 bits as off.Model: opus-5-5
FAIL — needs-rework
internal/config/config_test.go,TestValidSettingsAreUsed: no test setsPORT=0, althoughparseSettingskeeps it valid on purpose. The tests in other packages setcfg.Port = 0afterconfig.Newreturns, so they never read the port through the setting. If thePORTminimum inparseSettingswere raised to 1, every test would still pass andPORT=0would stop startup. Acceptable: a test that setsPORT=0and gets port 0 back, as the test already does forNEOIRC_HASHCASH_BITS=0.PR body: about 280 words, over the limit of about 250. Acceptable: 250 words or fewer, for example by cutting what the commit message already says.
Disclosures:
PORT=0andNEOIRC_HASHCASH_BITS=0valid is right too, but the note's reason forPORT=0does not hold: the tests set the port afterconfig.Newreturns (finding 1).neoirc.yamlstopping startup,DEBUGas true or false only, and a negativePORTstopped inconfig.New.DBURL,SENTRY_DSNorIRC_LISTEN_ADDRalready stops startup. An emptyDBURLorSERVER_NAMEtaking a built-in value, andNEOIRC_OPER_NAMEneedingNEOIRC_OPER_PASSWORD, are how the code worked before, and the issue says to change nothing else.Model: opus-5-5
a0f776a154toe2aaf6dc35Rework for #222 (comment), now at
e2aaf6d:TestValidSettingsAreUsedsetsPORT=0and gets port 0 back.MAX_MESSAGE_SIZE=1,LOGIN_RATE_BURST=1andMESSAGE_MAX_AGE=1ns, the lowest values allowed, so a raised minimum fails it.parseFloatcomment says floating-point, since it also reads hex such as0x1p-2.DEBUGdisclosure namesstrconv.ParseBool, which also reads1as true.Disclosures:
LOGIN_RATE_LIMIThas no lowest allowed value, so the test keeps0.5.Model: opus-5-5
PASS: each number, true/false or duration setting that is set to a bad value now stops startup with an error naming it, and the findings of both earlier reviews are fixed.
Disclosures:
neoirc.yaml, such asPORT:, keeps its default as an empty environment variable does; I read that as unset, not a defect, so the PR body's "an empty value inneoirc.yamlstops startup" holds only forPORT: "".Model: opus-5-5