Tracking issue for the non-blocking nits raised in the independent review of PR #92 (#80), so they do not become untracked. None blocked that merge.
Items
os.Unsetenv without restore.internal/config/env_test.go calls os.Unsetenv directly to simulate an unset variable. Unlike t.Setenv, that does not restore the prior value at test end, so it leaks unset state into later tests in the same binary. It exactly mirrors the pre-existing pattern in internal/config/config_test.go, which is why it was not a blocker. Fix both: capture and restore via t.Cleanup, or add a small helper that does.
config.New tests do not neutralise ambient environment. The config.New-level tests set only the variable under test. A developer with, say, DEBUG or PORT exported in their shell (or a .env file — note godotenv/autoload is imported by the config package) could get different results locally than CI. Explicitly clear the full set of variables the config reads at the start of those tests.
TestEnvPort error assertions are weaker than its siblings. The other helper tables assert the offending value appears in the error message; the envPort error branch only checks the sentinel via errors.Is. Make it consistent — the whole point of #80 is that the operator can see which key and which value were wrong.
ErrInvalidPort covers only the above-range case. Below-range values (0, negatives) surface as ErrNonPositiveValue from the inner envPositiveInt. Both correctly abort startup, so this is cosmetic, but a caller matching on ErrInvalidPort to mean "bad port" will miss half the bad ports. Either route both bounds through ErrInvalidPort or document the split.
README.md (~line 104) cites RETENTION_SWEEP_INTERVAL in prose but the variable is absent from the configuration table. Add it, with its default and the note that a set-but-unparseable value aborts startup.
Definition of done
Items 1-3 fixed; item 4 either fixed or documented; item 5 added to the table.
make check green via the repo's own entrypoints; .golangci.yml untouched.
Tracking issue for the non-blocking nits raised in the independent review of PR #92 (#80), so they do not become untracked. None blocked that merge.
## Items
1. **`os.Unsetenv` without restore.** `internal/config/env_test.go` calls `os.Unsetenv` directly to simulate an unset variable. Unlike `t.Setenv`, that does not restore the prior value at test end, so it leaks unset state into later tests in the same binary. It exactly mirrors the pre-existing pattern in `internal/config/config_test.go`, which is why it was not a blocker. Fix both: capture and restore via `t.Cleanup`, or add a small helper that does.
2. **`config.New` tests do not neutralise ambient environment.** The `config.New`-level tests set only the variable under test. A developer with, say, `DEBUG` or `PORT` exported in their shell (or a `.env` file — note `godotenv/autoload` is imported by the config package) could get different results locally than CI. Explicitly clear the full set of variables the config reads at the start of those tests.
3. **`TestEnvPort` error assertions are weaker than its siblings.** The other helper tables assert the offending value appears in the error message; the `envPort` error branch only checks the sentinel via `errors.Is`. Make it consistent — the whole point of #80 is that the operator can see which key and which value were wrong.
4. **`ErrInvalidPort` covers only the above-range case.** Below-range values (`0`, negatives) surface as `ErrNonPositiveValue` from the inner `envPositiveInt`. Both correctly abort startup, so this is cosmetic, but a caller matching on `ErrInvalidPort` to mean "bad port" will miss half the bad ports. Either route both bounds through `ErrInvalidPort` or document the split.
5. **`README.md` (~line 104) cites `RETENTION_SWEEP_INTERVAL`** in prose but the variable is absent from the configuration table. Add it, with its default and the note that a set-but-unparseable value aborts startup.
## Definition of done
- Items 1-3 fixed; item 4 either fixed or documented; item 5 added to the table.
- `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.
Plan. The items are from August; check each against next first and fix only what is still true, saying in the PR which items were already settled.
Every test that unsets a variable restores it afterwards (t.Setenv and a cleanup), in both config test files.
The config.New tests clear every variable the config reads before setting the one under test, and do not read a .env file.
TestEnvPort's error cases check the offending value appears in the message, as its siblings do.
A port of zero or below and a port above the range both match ErrInvalidPort.
RETENTION_SWEEP_INTERVAL is in the README configuration table with its default and the note that an unparseable value stops startup, and the Settings page, which uses the table's wording, agrees.
Model: opus-5-5
Plan. The items are from August; check each against `next` first and fix only what is still true, saying in the PR which items were already settled.
1. Every test that unsets a variable restores it afterwards (`t.Setenv` and a cleanup), in both config test files.
2. The `config.New` tests clear every variable the config reads before setting the one under test, and do not read a `.env` file.
3. `TestEnvPort`'s error cases check the offending value appears in the message, as its siblings do.
4. A port of zero or below and a port above the range both match `ErrInvalidPort`.
5. `RETENTION_SWEEP_INTERVAL` is in the README configuration table with its default and the note that an unparseable value stops startup, and the Settings page, which uses the table's wording, agrees.
Model: opus-5-5
Built in #444. (Edited after its review to match the reworked change.)
Config tests, and the first-boot debug log test that builds a Config, start from an empty environment and restore it when they end, so the developer's shell cannot change the result; no list of variables is kept. TestEnvPort checks that the bad value appears in its errors. Every out-of-range PORT, including one too large or too small for an int, now matches ErrInvalidPort. The README table and the Settings page say a RETENTION_SWEEP_INTERVAL that does not parse, or is zero or negative, fails startup.
Already settled on next: the .env half of item 2 (the config package no longer reads .env on import), and the table row and default for item 5.
Rule suppressed: paralleltest on TestEnvPositiveInt and TestEnvPort, which set the environment inside their shared runner.
Model: opus-5-5
Built in https://git.eeqj.de/sneak/webhooker/pulls/444. (Edited after its review to match the reworked change.)
Config tests, and the first-boot debug log test that builds a Config, start from an empty environment and restore it when they end, so the developer's shell cannot change the result; no list of variables is kept. `TestEnvPort` checks that the bad value appears in its errors. Every out-of-range `PORT`, including one too large or too small for an int, now matches `ErrInvalidPort`. The README table and the Settings page say a `RETENTION_SWEEP_INTERVAL` that does not parse, or is zero or negative, fails startup.
Already settled on `next`: the `.env` half of item 2 (the config package no longer reads `.env` on import), and the table row and default for item 5.
- Rule suppressed: `paralleltest` on `TestEnvPositiveInt` and `TestEnvPort`, which set the environment inside their shared runner.
Model: opus-5-5
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.
Tracking issue for the non-blocking nits raised in the independent review of PR #92 (#80), so they do not become untracked. None blocked that merge.
Items
os.Unsetenvwithout restore.internal/config/env_test.gocallsos.Unsetenvdirectly to simulate an unset variable. Unliket.Setenv, that does not restore the prior value at test end, so it leaks unset state into later tests in the same binary. It exactly mirrors the pre-existing pattern ininternal/config/config_test.go, which is why it was not a blocker. Fix both: capture and restore viat.Cleanup, or add a small helper that does.config.Newtests do not neutralise ambient environment. Theconfig.New-level tests set only the variable under test. A developer with, say,DEBUGorPORTexported in their shell (or a.envfile — notegodotenv/autoloadis imported by the config package) could get different results locally than CI. Explicitly clear the full set of variables the config reads at the start of those tests.TestEnvPorterror assertions are weaker than its siblings. The other helper tables assert the offending value appears in the error message; theenvPorterror branch only checks the sentinel viaerrors.Is. Make it consistent — the whole point of #80 is that the operator can see which key and which value were wrong.ErrInvalidPortcovers only the above-range case. Below-range values (0, negatives) surface asErrNonPositiveValuefrom the innerenvPositiveInt. Both correctly abort startup, so this is cosmetic, but a caller matching onErrInvalidPortto mean "bad port" will miss half the bad ports. Either route both bounds throughErrInvalidPortor document the split.README.md(~line 104) citesRETENTION_SWEEP_INTERVALin prose but the variable is absent from the configuration table. Add it, with its default and the note that a set-but-unparseable value aborts startup.Definition of done
make checkgreen via the repo's own entrypoints;.golangci.ymluntouched.Plan. The items are from August; check each against
nextfirst and fix only what is still true, saying in the PR which items were already settled.t.Setenvand a cleanup), in both config test files.config.Newtests clear every variable the config reads before setting the one under test, and do not read a.envfile.TestEnvPort's error cases check the offending value appears in the message, as its siblings do.ErrInvalidPort.RETENTION_SWEEP_INTERVALis in the README configuration table with its default and the note that an unparseable value stops startup, and the Settings page, which uses the table's wording, agrees.Model: opus-5-5
Built in #444. (Edited after its review to match the reworked change.)
Config tests, and the first-boot debug log test that builds a Config, start from an empty environment and restore it when they end, so the developer's shell cannot change the result; no list of variables is kept.
TestEnvPortchecks that the bad value appears in its errors. Every out-of-rangePORT, including one too large or too small for an int, now matchesErrInvalidPort. The README table and the Settings page say aRETENTION_SWEEP_INTERVALthat does not parse, or is zero or negative, fails startup.Already settled on
next: the.envhalf of item 2 (the config package no longer reads.envon import), and the table row and default for item 5.paralleltestonTestEnvPositiveIntandTestEnvPort, which set the environment inside their shared runner.Model: opus-5-5