Found by the final integration review of #111, plus one item from the review of #153. Blocks the 1.0.0 tag: publishing this README ships false statements about the product.
1. The lockout warning does not fire in the default environment
Added by #149 and gated on IsProd(). But WEBHOOKER_ENVIRONMENT defaults to dev, so an internet-exposed deployment whose operator never set that variable gets NO warning, despite having exactly the shared-bucket exposure the warning exists to announce. "Forgot to set the env var" is precisely the operator-error class this is meant to catch, so gating on another env var being set correctly defeats it.
That gating came from #149's own definition of done, which said "in production mode" — the scoping was wrong, not the implementation.
Fix: emit the warning whenever TRUSTED_PROXIES is empty, regardless of environment. Word it so it is accurate in both cases — behind a proxy it means shared buckets and a remotely deniable admin login; with no proxy in front (a plausible dev setup) it is harmless and the text should not imply otherwise. Extend the existing test so the dev-with-empty case now asserts the warning IS emitted.
2. The Docker build section describes a build that does not exist
README.md:1131-1135 describes two stages, golang:1.24, and a builder that "installs golangci-lint" and "runs make check".
Actual Dockerfile: three stages — a golangci/golangci-lint:v2.12.2 lint stage running make fmt-check and make lint, and a golang:1.26.1-bookworm builder running make test and make build. It contradicts the passage fifteen lines below it added by #119, and describes the single-builder shape REPO_POLICIES.md:102-106 forbids.
3. The receiver endpoint's accepted methods are wrong
README.md:917 documents the public webhook endpoint as ANY ... (accepts all methods). internal/handlers/webhook.go:24-33 returns 405 for everything except POST. README.md:514 compounds it with "HTTP method (POST, PUT, etc.)". This is the one endpoint the public interacts with.
4. The last unqualified per-IP rate-limit claim
README.md:326-327 still reads "go-chi/httprate for per-IP login rate limiting" — the exact unconditional claim #149 corrected everywhere else. Qualify it consistently with the rest.
5. A configuration section is orphaned under the wrong heading
README.md:148-166 documents session expiry but sits inside the #### Trusted proxies subsection: #66 placed it under ### Configuration, then #88 inserted a heading above it. Visible only once both landed.
6. SESSION_IDLE_TIMEOUT is missing from the startup summary
internal/config/config.go logs the effective configuration at startup but omits sessionIdleTimeout. It is the one variable where a perfectly VALID value (any non-positive number) silently disables a security control.
Future Steps lists "Password change and reset flow" while Completed Steps records #65 as landed. The change flow exists; reset does not — split the entry rather than deleting it.
8. TODO.md overclaims CI verification
The Status paragraph says next is "verified green both by CI and by cache-defeated container runs". The cache-defeated half is true and is the stronger evidence; the CI half is not currently supported (#152). Claim only what is true.
Definition of done
Every statement above matches the code.
SWEEP THE WHOLE README rather than fixing these lines only. #141 failed review for fixing exactly the lines it was given, and its successor failed again for the same reason. Verify every documented HTTP route's methods against internal/server/routes.go, and the build/deployment section against the actual Dockerfile and workflow.
Report the complete list of statements you checked, including those you judged correct.
Implementation requirements
This unit MAY edit TODO.md for items 7 and 8 — the documented exception to #112.
Do NOT change rate-limit keying, limits, or the TRUSTED_PROXIES default — that is #150, an owner decision. Item 1 changes only WHEN the warning fires and how it is worded.
Branch from next, PR based on next, single commit, title ending (closes #N).
Gate on make check plus the Docker lint path with the cache defeated.
Found by the final integration review of https://git.eeqj.de/sneak/webhooker/pulls/111, plus one item from the review of https://git.eeqj.de/sneak/webhooker/pulls/153. Blocks the 1.0.0 tag: publishing this README ships false statements about the product.
## 1. The lockout warning does not fire in the default environment
Added by https://git.eeqj.de/sneak/webhooker/issues/149 and gated on `IsProd()`. But `WEBHOOKER_ENVIRONMENT` defaults to `dev`, so an internet-exposed deployment whose operator never set that variable gets NO warning, despite having exactly the shared-bucket exposure the warning exists to announce. "Forgot to set the env var" is precisely the operator-error class this is meant to catch, so gating on another env var being set correctly defeats it.
That gating came from https://git.eeqj.de/sneak/webhooker/issues/149's own definition of done, which said "in production mode" — the scoping was wrong, not the implementation.
Fix: emit the warning whenever `TRUSTED_PROXIES` is empty, regardless of environment. Word it so it is accurate in both cases — behind a proxy it means shared buckets and a remotely deniable admin login; with no proxy in front (a plausible dev setup) it is harmless and the text should not imply otherwise. Extend the existing test so the dev-with-empty case now asserts the warning IS emitted.
## 2. The Docker build section describes a build that does not exist
`README.md:1131-1135` describes two stages, `golang:1.24`, and a builder that "installs golangci-lint" and "runs `make check`".
Actual `Dockerfile`: three stages — a `golangci/golangci-lint:v2.12.2` lint stage running `make fmt-check` and `make lint`, and a `golang:1.26.1-bookworm` builder running `make test` and `make build`. It contradicts the passage fifteen lines below it added by https://git.eeqj.de/sneak/webhooker/issues/119, and describes the single-builder shape `REPO_POLICIES.md:102-106` forbids.
## 3. The receiver endpoint's accepted methods are wrong
`README.md:917` documents the public webhook endpoint as `ANY ... (accepts all methods)`. `internal/handlers/webhook.go:24-33` returns 405 for everything except POST. `README.md:514` compounds it with "HTTP method (POST, PUT, etc.)". This is the one endpoint the public interacts with.
## 4. The last unqualified per-IP rate-limit claim
`README.md:326-327` still reads "**go-chi/httprate** for per-IP login rate limiting" — the exact unconditional claim https://git.eeqj.de/sneak/webhooker/issues/149 corrected everywhere else. Qualify it consistently with the rest.
## 5. A configuration section is orphaned under the wrong heading
`README.md:148-166` documents session expiry but sits inside the `#### Trusted proxies` subsection: https://git.eeqj.de/sneak/webhooker/issues/66 placed it under `### Configuration`, then https://git.eeqj.de/sneak/webhooker/issues/88 inserted a heading above it. Visible only once both landed.
## 6. `SESSION_IDLE_TIMEOUT` is missing from the startup summary
`internal/config/config.go` logs the effective configuration at startup but omits `sessionIdleTimeout`. It is the one variable where a perfectly VALID value (any non-positive number) silently disables a security control.
## 7. `TODO.md` Future Steps contradicts Completed Steps
Future Steps lists "Password change and reset flow" while Completed Steps records https://git.eeqj.de/sneak/webhooker/issues/65 as landed. The change flow exists; reset does not — split the entry rather than deleting it.
## 8. `TODO.md` overclaims CI verification
The Status paragraph says `next` is "verified green both by CI and by cache-defeated container runs". The cache-defeated half is true and is the stronger evidence; the CI half is not currently supported (https://git.eeqj.de/sneak/webhooker/issues/152). Claim only what is true.
## Definition of done
- Every statement above matches the code.
- SWEEP THE WHOLE README rather than fixing these lines only. https://git.eeqj.de/sneak/webhooker/issues/141 failed review for fixing exactly the lines it was given, and its successor failed again for the same reason. Verify every documented HTTP route's methods against `internal/server/routes.go`, and the build/deployment section against the actual `Dockerfile` and workflow.
- Report the complete list of statements you checked, including those you judged correct.
## Implementation requirements
- This unit MAY edit `TODO.md` for items 7 and 8 — the documented exception to https://git.eeqj.de/sneak/webhooker/issues/112.
- Do NOT change rate-limit keying, limits, or the `TRUSTED_PROXIES` default — that is https://git.eeqj.de/sneak/webhooker/issues/150, an owner decision. Item 1 changes only WHEN the warning fires and how it is worded.
- Branch from `next`, PR based on `next`, single commit, title ending ` (closes #N)`.
- Gate on `make check` plus the Docker lint path with the cache defeated.
clawbot
added this to the 1.0.0 milestone 2026-08-12 13:32:46 +02:00
clawbot
self-assigned this 2026-08-12 13:32:46 +02:00
clawbot
changed title from Release-blocking README inaccuracies: the Docker build section and the receiver endpoint's accepted methods to Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environment2026-08-12 13:50:03 +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.
Found by the final integration review of #111, plus one item from the review of #153. Blocks the 1.0.0 tag: publishing this README ships false statements about the product.
1. The lockout warning does not fire in the default environment
Added by #149 and gated on
IsProd(). ButWEBHOOKER_ENVIRONMENTdefaults todev, so an internet-exposed deployment whose operator never set that variable gets NO warning, despite having exactly the shared-bucket exposure the warning exists to announce. "Forgot to set the env var" is precisely the operator-error class this is meant to catch, so gating on another env var being set correctly defeats it.That gating came from #149's own definition of done, which said "in production mode" — the scoping was wrong, not the implementation.
Fix: emit the warning whenever
TRUSTED_PROXIESis empty, regardless of environment. Word it so it is accurate in both cases — behind a proxy it means shared buckets and a remotely deniable admin login; with no proxy in front (a plausible dev setup) it is harmless and the text should not imply otherwise. Extend the existing test so the dev-with-empty case now asserts the warning IS emitted.2. The Docker build section describes a build that does not exist
README.md:1131-1135describes two stages,golang:1.24, and a builder that "installs golangci-lint" and "runsmake check".Actual
Dockerfile: three stages — agolangci/golangci-lint:v2.12.2lint stage runningmake fmt-checkandmake lint, and agolang:1.26.1-bookwormbuilder runningmake testandmake build. It contradicts the passage fifteen lines below it added by #119, and describes the single-builder shapeREPO_POLICIES.md:102-106forbids.3. The receiver endpoint's accepted methods are wrong
README.md:917documents the public webhook endpoint asANY ... (accepts all methods).internal/handlers/webhook.go:24-33returns 405 for everything except POST.README.md:514compounds it with "HTTP method (POST, PUT, etc.)". This is the one endpoint the public interacts with.4. The last unqualified per-IP rate-limit claim
README.md:326-327still reads "go-chi/httprate for per-IP login rate limiting" — the exact unconditional claim #149 corrected everywhere else. Qualify it consistently with the rest.5. A configuration section is orphaned under the wrong heading
README.md:148-166documents session expiry but sits inside the#### Trusted proxiessubsection: #66 placed it under### Configuration, then #88 inserted a heading above it. Visible only once both landed.6.
SESSION_IDLE_TIMEOUTis missing from the startup summaryinternal/config/config.gologs the effective configuration at startup but omitssessionIdleTimeout. It is the one variable where a perfectly VALID value (any non-positive number) silently disables a security control.7.
TODO.mdFuture Steps contradicts Completed StepsFuture Steps lists "Password change and reset flow" while Completed Steps records #65 as landed. The change flow exists; reset does not — split the entry rather than deleting it.
8.
TODO.mdoverclaims CI verificationThe Status paragraph says
nextis "verified green both by CI and by cache-defeated container runs". The cache-defeated half is true and is the stronger evidence; the CI half is not currently supported (#152). Claim only what is true.Definition of done
internal/server/routes.go, and the build/deployment section against the actualDockerfileand workflow.Implementation requirements
TODO.mdfor items 7 and 8 — the documented exception to #112.TRUSTED_PROXIESdefault — that is #150, an owner decision. Item 1 changes only WHEN the warning fires and how it is worded.next, PR based onnext, single commit, title ending(closes #N).make checkplus the Docker lint path with the cache defeated.Release-blocking README inaccuracies: the Docker build section and the receiver endpoint's accepted methodsto Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environmentclawbot referenced this issue2026-08-17 23:50:11 +02:00