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