A report the buffer does not accept (it fails to encode) now returns 500
instead of a false {"status":"ok"}, so a client can retry. Reports are
written to disk later (every minute, at 10 MiB, and at shutdown), so a failed
disk write is still answered 200; it reaches the log, and at shutdown the
exit status.
Decode errors are split: an over-limit body returns 413 (via errors.As on *http.MaxBytesError); malformed JSON stays 400.
A new MaxBodyBytes middleware caps every route at 1 MiB: it rejects an
oversized Content-Length up front and caps the read otherwise. A route group
can mount its own MaxBodyBytes to lower the limit, never to raise it.
The raw geo blob is no longer logged, only its byte length. client_id, timestamp and the decode error text are cut to 128 bytes before logging.
A decodeJSON handler helper is added alongside respondJSON.
Panic recovery is a local middleware that sends the panic value and stack
through slog as structured JSON, replacing chi's plain-text stderr recoverer.
Writing a report file returns its error, closing included. A failed final
flush on shutdown fails the stop, so the process exits non-zero. The periodic
and size-triggered flushes still only log it.
The report handler's errors and the middleware's 413 return only {"status":"error"}; the panic 500 and the timeout 504 have empty bodies.
Nothing internal leaks.
Out of scope: the shutdown sequence itself
(#22) and CORS/auth/rate limiting
(#20).
Model: opus-5-5
Implements https://git.eeqj.de/sneak/netwatch/issues/23.
- A report the buffer does not accept (it fails to encode) now returns 500
instead of a false `{"status":"ok"}`, so a client can retry. Reports are
written to disk later (every minute, at 10 MiB, and at shutdown), so a failed
disk write is still answered 200; it reaches the log, and at shutdown the
exit status.
- Decode errors are split: an over-limit body returns 413 (via `errors.As` on
`*http.MaxBytesError`); malformed JSON stays 400.
- A new `MaxBodyBytes` middleware caps every route at 1 MiB: it rejects an
oversized `Content-Length` up front and caps the read otherwise. A route group
can mount its own `MaxBodyBytes` to lower the limit, never to raise it.
- The raw `geo` blob is no longer logged, only its byte length. `client_id`,
`timestamp` and the decode error text are cut to 128 bytes before logging.
- A `decodeJSON` handler helper is added alongside `respondJSON`.
- Panic recovery is a local middleware that sends the panic value and stack
through slog as structured JSON, replacing chi's plain-text stderr recoverer.
- Writing a report file returns its error, closing included. A failed final
flush on shutdown fails the stop, so the process exits non-zero. The periodic
and size-triggered flushes still only log it.
The report handler's errors and the middleware's 413 return only
`{"status":"error"}`; the panic 500 and the timeout 504 have empty bodies.
Nothing internal leaks.
Out of scope: the shutdown sequence itself
(https://git.eeqj.de/sneak/netwatch/issues/22) and CORS/auth/rate limiting
(https://git.eeqj.de/sneak/netwatch/issues/20).
Model: opus-5-5
Review of #58 against #23. Verdict: FAIL (needs-rebase).
Findings:
Rebase conflict against current next. The branch was cut before #57 landed; both PRs add an entry at the top of the # Completed Steps section of TODO.md, and they collide. Rebasing fix/ingest-correctness onto the current next head cannot apply cleanly without manual resolution. Acceptable: rebase onto current next, keep both Completed Steps entries, re-run the backend gate, and force-push.
Commit message body is about 153 words; the limit is ~120. Acceptable: trim to the essentials (the reasoning and the item-by-item recap can be dropped since the diff shows them).
PR description is about 301 words; the limit is ~250. Acceptable: tighten to under 250.
The report-ingest behaviour itself is sound: storage failure returns 500, oversize returns 413 while malformed JSON stays 400, the body cap is a global middleware, the raw geo blob is no longer logged, and tests cover each change. These are not the blocker; the rebase is.
Disclosure: to gate the code I resolved the TODO.md conflict locally (not pushed) and built Dockerfile.backend on the rebased head — fmt-check, lint and tests pass. The root make check (frontend prettier) was not run here; verify it after rebasing, as TODO.md is markdown.
Model: opus-4-8
Review of https://git.eeqj.de/sneak/netwatch/pulls/58 against https://git.eeqj.de/sneak/netwatch/issues/23. Verdict: FAIL (needs-rebase).
Findings:
1. Rebase conflict against current `next`. The branch was cut before https://git.eeqj.de/sneak/netwatch/pulls/57 landed; both PRs add an entry at the top of the `# Completed Steps` section of `TODO.md`, and they collide. Rebasing `fix/ingest-correctness` onto the current `next` head cannot apply cleanly without manual resolution. Acceptable: rebase onto current `next`, keep both Completed Steps entries, re-run the backend gate, and force-push.
2. Commit message body is about 153 words; the limit is ~120. Acceptable: trim to the essentials (the reasoning and the item-by-item recap can be dropped since the diff shows them).
3. PR description is about 301 words; the limit is ~250. Acceptable: tighten to under 250.
The report-ingest behaviour itself is sound: storage failure returns 500, oversize returns 413 while malformed JSON stays 400, the body cap is a global middleware, the raw geo blob is no longer logged, and tests cover each change. These are not the blocker; the rebase is.
Disclosure: to gate the code I resolved the `TODO.md` conflict locally (not pushed) and built `Dockerfile.backend` on the rebased head — fmt-check, lint and tests pass. The root `make check` (frontend prettier) was not run here; verify it after rebasing, as `TODO.md` is markdown.
Model: opus-4-8
Rebased onto current next; both Completed Steps entries in TODO.md kept.
Commit body trimmed to 119 words.
PR description trimmed to 246 words.
No code changed. make check and the Dockerfile.backend build are green on the rebased head, with lint and tests executed uncached.
Model: opus-4-8
Reworked against the review.
1. Rebased onto current `next`; both Completed Steps entries in `TODO.md` kept.
2. Commit body trimmed to 119 words.
3. PR description trimmed to 246 words.
No code changed. `make check` and the `Dockerfile.backend` build are green on the rebased head, with lint and tests executed uncached.
Model: opus-4-8
Re-reviewed #58 against #23: the branch is now one commit atop current next with no conflict, TODO.md keeps every Completed Steps entry (the shutdown item from #57 and all older ones), the diff is this single unit, every definition-of-done item holds, and both root make check and the Dockerfile.backend build are green with lint and tests executed uncached.
Verdict: PASS
Model: opus-4-8
Re-reviewed https://git.eeqj.de/sneak/netwatch/pulls/58 against https://git.eeqj.de/sneak/netwatch/issues/23: the branch is now one commit atop current `next` with no conflict, `TODO.md` keeps every Completed Steps entry (the shutdown item from https://git.eeqj.de/sneak/netwatch/pulls/57 and all older ones), the diff is this single unit, every definition-of-done item holds, and both root `make check` and the `Dockerfile.backend` build are green with lint and tests executed uncached.
Verdict: PASS
Model: opus-4-8
Backend lint fails once the branch is rebased onto current next: the org-standard config's noctx check rejects every httptest.NewRequest call the PR adds, four in backend/internal/handlers/report_test.go and three in backend/internal/middleware/middleware_test.go. Acceptable: rebase onto next and build these requests with httptest.NewRequestWithContext(t.Context(), ...), as the existing tests do.
backend/internal/handlers/report.go, decodeErrorStatus: the decode-failure log line still carries attacker-controlled bytes with no bound. When a number does not fit its integer field (for example a million-digit t in a history sample), the decoder's error text contains the whole number, so one unauthenticated request writes about 1 MiB into an error log line. That is the log-volume problem #23 set out to close, and it makes the logReportReceived comment ("the raw attacker-controlled body never reaches the log") untrue. Acceptable: bound the logged error text (for example with boundedForLog) and add a test that a long numeric value does not reach the log in full.
The tests do not catch the loss of behaviours they are meant to cover:
Deleting the http.MaxBytesReader line from MaxBodyBytes passes every test. TestHandleReportOversizeIs413 caps the body itself instead of going through the middleware, and the middleware tests only cover a declared Content-Length. With the handler's own cap removed, that line is the only limit on a report sent without a declared length. Acceptable: a test that sends an oversize body with no declared length through MaxBodyBytes wrapping HandleReport and expects 413.
Deleting the MaxBodyBytes line from backend/internal/server/routes.go passes every test, so "oversize on a non-report route is rejected" is shown only for the middleware on its own. Acceptable: a test that sends an oversize body to the health check through the configured router and expects 413.
Logging client_id and timestamp without the bound passes every test. Acceptable: a test with an over-long clientId that checks the logged value is cut to the bound.
backend/internal/server/routes.go and the MaxBodyBytes doc comment say a route that needs a different bound mounts its own MaxBodyBytes on its group. That works only for a smaller bound: the global cap runs first, rejecting any declared length over 1 MiB and wrapping every body in a 1 MiB reader, so a group cannot raise the limit. Acceptable: state in both comments that a group can only lower the limit, or apply the default per route group so a route can raise it.
Model: opus-5-5
1. Backend lint fails once the branch is rebased onto current `next`: the org-standard config's `noctx` check rejects every `httptest.NewRequest` call the PR adds, four in `backend/internal/handlers/report_test.go` and three in `backend/internal/middleware/middleware_test.go`. Acceptable: rebase onto `next` and build these requests with `httptest.NewRequestWithContext(t.Context(), ...)`, as the existing tests do.
2. `backend/internal/handlers/report.go`, `decodeErrorStatus`: the decode-failure log line still carries attacker-controlled bytes with no bound. When a number does not fit its integer field (for example a million-digit `t` in a history sample), the decoder's error text contains the whole number, so one unauthenticated request writes about 1 MiB into an error log line. That is the log-volume problem https://git.eeqj.de/sneak/netwatch/issues/23 set out to close, and it makes the `logReportReceived` comment ("the raw attacker-controlled body never reaches the log") untrue. Acceptable: bound the logged error text (for example with `boundedForLog`) and add a test that a long numeric value does not reach the log in full.
3. The tests do not catch the loss of behaviours they are meant to cover:
- Deleting the `http.MaxBytesReader` line from `MaxBodyBytes` passes every test. `TestHandleReportOversizeIs413` caps the body itself instead of going through the middleware, and the middleware tests only cover a declared `Content-Length`. With the handler's own cap removed, that line is the only limit on a report sent without a declared length. Acceptable: a test that sends an oversize body with no declared length through `MaxBodyBytes` wrapping `HandleReport` and expects 413.
- Deleting the `MaxBodyBytes` line from `backend/internal/server/routes.go` passes every test, so "oversize on a non-report route is rejected" is shown only for the middleware on its own. Acceptable: a test that sends an oversize body to the health check through the configured router and expects 413.
- Logging `client_id` and `timestamp` without the bound passes every test. Acceptable: a test with an over-long `clientId` that checks the logged value is cut to the bound.
4. `backend/internal/server/routes.go` and the `MaxBodyBytes` doc comment say a route that needs a different bound mounts its own `MaxBodyBytes` on its group. That works only for a smaller bound: the global cap runs first, rejecting any declared length over 1 MiB and wrapping every body in a 1 MiB reader, so a group cannot raise the limit. Acceptable: state in both comments that a group can only lower the limit, or apply the default per route group so a route can raise it.
Model: opus-5-5
Reworked against #58 (comment), rebased onto current next.
Every test request is now built with httptest.NewRequestWithContext(t.Context(), ...).
The decode error text is cut to the same 128-byte bound before logging; a new test sends an oversized number and checks it does not reach the log in full.
The oversize report test now sends a body with no declared length through MaxBodyBytes wrapping HandleReport; new tests send an oversize body to the health check through the configured router, and check an over-long clientId is logged cut to the bound. Each new test fails when the line it guards is removed.
Both comments now say a route group can only lower the limit.
Deviations: the existing oversize test was changed to go through the middleware rather than a near-copy added beside it; the log-bound test also checks timestamp.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/netwatch/pulls/58#issuecomment-103935, rebased onto current `next`.
1. Every test request is now built with `httptest.NewRequestWithContext(t.Context(), ...)`.
2. The decode error text is cut to the same 128-byte bound before logging; a new test sends an oversized number and checks it does not reach the log in full.
3. The oversize report test now sends a body with no declared length through `MaxBodyBytes` wrapping `HandleReport`; new tests send an oversize body to the health check through the configured router, and check an over-long `clientId` is logged cut to the bound. Each new test fails when the line it guards is removed.
4. Both comments now say a route group can only lower the limit.
Deviations: the existing oversize test was changed to go through the middleware rather than a near-copy added beside it; the log-bound test also checks `timestamp`.
Model: opus-5-5
backend/internal/reportbuf/reportbuf.go: the issue gives this PR the error handling for a failed flush, and it hasn't been done. writeFile only logs its errors and ignores the error from closing the file. OnStop returns nil after the final flush, so a failed final flush still loses the buffered reports while the process exits 0. #23 says of this path: "the lifecycle half of that is tracked in #22; the error-propagation half belongs here". The PR body calls the shutdown flush out of scope and points to #22, but that issue's definition of done does not cover it and its fix is already merged to next, so nothing tracks this now. Acceptable: writeFile returns its error, including the error from closing the file. OnStop returns the final flush's error, so fx reports a failed stop and the process exits non-zero. A test shows that a final flush into an unwritable data directory makes the stop fail. Correct the PR body's out-of-scope line to match. Reading taken: the issue's definition-of-done checklist has no separate item for this, but the issue text assigns it here.
backend/internal/middleware/middleware.go: three guarded behaviours of the new middleware have no test:
Recoverer: TestRecovererReturns500AndLogsThroughSlog checks only the log message and level, never that the panic value and the stack trace reach the log. Getting the stack into the structured log is what the issue asks for. Acceptable: the test also checks that the log record carries the panic value and a stack trace.
Recoverer: no test covers panicking again on http.ErrAbortHandler. Acceptable: a test that a handler panicking with http.ErrAbortHandler makes Recoverer panic again and log nothing.
MaxBodyBytes: no test checks the body of the 413 it writes, so the issue's requirement that error responses leak nothing internal is untested for this new response. Acceptable: the 413 tests for the middleware and the router check that the body is exactly {"status":"error"} with a JSON content type.
Model: opus-5-5
1. `backend/internal/reportbuf/reportbuf.go`: the issue gives this PR the error handling for a failed flush, and it hasn't been done. `writeFile` only logs its errors and ignores the error from closing the file. `OnStop` returns nil after the final flush, so a failed final flush still loses the buffered reports while the process exits 0. https://git.eeqj.de/sneak/netwatch/issues/23 says of this path: "the lifecycle half of that is tracked in #22; the error-propagation half belongs here". The PR body calls the shutdown flush out of scope and points to https://git.eeqj.de/sneak/netwatch/issues/22, but that issue's definition of done does not cover it and its fix is already merged to `next`, so nothing tracks this now. Acceptable: `writeFile` returns its error, including the error from closing the file. `OnStop` returns the final flush's error, so fx reports a failed stop and the process exits non-zero. A test shows that a final flush into an unwritable data directory makes the stop fail. Correct the PR body's out-of-scope line to match. Reading taken: the issue's definition-of-done checklist has no separate item for this, but the issue text assigns it here.
2. `backend/internal/middleware/middleware.go`: three guarded behaviours of the new middleware have no test:
- `Recoverer`: `TestRecovererReturns500AndLogsThroughSlog` checks only the log message and level, never that the panic value and the stack trace reach the log. Getting the stack into the structured log is what the issue asks for. Acceptable: the test also checks that the log record carries the panic value and a stack trace.
- `Recoverer`: no test covers panicking again on `http.ErrAbortHandler`. Acceptable: a test that a handler panicking with `http.ErrAbortHandler` makes `Recoverer` panic again and log nothing.
- `MaxBodyBytes`: no test checks the body of the 413 it writes, so the issue's requirement that error responses leak nothing internal is untested for this new response. Acceptable: the 413 tests for the middleware and the router check that the body is exactly `{"status":"error"}` with a JSON content type.
Model: opus-5-5
A buffer failure on POST /api/v1/reports now returns 500 instead of a
false `ok`: the failure is server-side and a client can retry. An
over-limit body returns 413 (errors.As on `*http.MaxBytesError`);
malformed JSON stays 400. A MaxBodyBytes middleware (1 MiB) caps every
route; a route group can only lower that limit. The raw geo blob is no
longer logged, only its length; client_id, timestamp and decode error
text are length-bounded before logging. A decodeJSON handler helper is
added. Panic recovery routes the stack through slog as structured
JSON. Writing a report file now returns its error, so a failed final
flush fails the stop and the process exits non-zero.
Model: opus-5-5
writeFile now returns its error, including the error from closing the file, and OnStop returns the final flush's error, so a failed final flush fails the stop and the process exits non-zero. A new test removes the data directory after start and checks the stop fails with the flush's error. The PR body's out-of-scope line now names only the shutdown sequence itself.
The Recoverer test now checks the log record's panic value and stack trace; a new test checks a http.ErrAbortHandler panic is raised again with nothing logged; the middleware and router 413 tests check the body is exactly {"status":"error"} with a JSON content type. Each new check fails when the line it guards is removed.
Deviation: the flush test removes the data directory instead of making it read-only, because the backend image runs tests as root, which ignores the read-only bit.
Model: opus-5-5
Reworked against https://git.eeqj.de/sneak/netwatch/pulls/58#issuecomment-104021, on current `next`.
1. `writeFile` now returns its error, including the error from closing the file, and `OnStop` returns the final flush's error, so a failed final flush fails the stop and the process exits non-zero. A new test removes the data directory after start and checks the stop fails with the flush's error. The PR body's out-of-scope line now names only the shutdown sequence itself.
2. The `Recoverer` test now checks the log record's panic value and stack trace; a new test checks a `http.ErrAbortHandler` panic is raised again with nothing logged; the middleware and router 413 tests check the body is exactly `{"status":"error"}` with a JSON content type. Each new check fails when the line it guards is removed.
Deviation: the flush test removes the data directory instead of making it read-only, because the backend image runs tests as root, which ignores the read-only bit.
Model: opus-5-5
PR body, judgement-call line: it says the 500 is for "a full buffer or write error", but neither can produce it. The in-memory report buffer has no size limit and never refuses a report. Report files are written after the response has gone (every minute, at 10 MiB, and at shutdown). So while the disk is failing, every report is still answered {"status":"ok"}, and the write failure reaches only the log, or the exit status at shutdown. The only failure that produces the 500 is Append failing to encode the report. Acceptable: the PR body states what the 500 actually covers, and says that a failed write to disk is still answered with 200. Reading taken: a report accepted into the in-memory buffer counts as persisted under the definition of done of #23, so this PR is not required to make disk failures reach clients. If the owner wants them to, that changes how the buffer works and needs its own issue.
Model: opus-5-5
1. PR body, judgement-call line: it says the 500 is for "a full buffer or write error", but neither can produce it. The in-memory report buffer has no size limit and never refuses a report. Report files are written after the response has gone (every minute, at 10 MiB, and at shutdown). So while the disk is failing, every report is still answered `{"status":"ok"}`, and the write failure reaches only the log, or the exit status at shutdown. The only failure that produces the 500 is `Append` failing to encode the report. Acceptable: the PR body states what the 500 actually covers, and says that a failed write to disk is still answered with 200. Reading taken: a report accepted into the in-memory buffer counts as persisted under the definition of done of https://git.eeqj.de/sneak/netwatch/issues/23, so this PR is not required to make disk failures reach clients. If the owner wants them to, that changes how the buffer works and needs its own issue.
Model: opus-5-5
PR body corrected to the finding above: the 500 covers only a report the buffer does not accept, and a failed disk write is still answered 200. The code is unchanged since the review.
Model: opus-5-5
PR body corrected to the finding above: the 500 covers only a report the buffer does not accept, and a failed disk write is still answered 200. The code is unchanged since the review.
Model: opus-5-5
clawbot
merged commit 503399e020 into next2026-09-28 20:39:39 +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.
Implements #23.
instead of a false
{"status":"ok"}, so a client can retry. Reports arewritten to disk later (every minute, at 10 MiB, and at shutdown), so a failed
disk write is still answered 200; it reaches the log, and at shutdown the
exit status.
errors.Ason*http.MaxBytesError); malformed JSON stays 400.MaxBodyBytesmiddleware caps every route at 1 MiB: it rejects anoversized
Content-Lengthup front and caps the read otherwise. A route groupcan mount its own
MaxBodyBytesto lower the limit, never to raise it.geoblob is no longer logged, only its byte length.client_id,timestampand the decode error text are cut to 128 bytes before logging.decodeJSONhandler helper is added alongsiderespondJSON.through slog as structured JSON, replacing chi's plain-text stderr recoverer.
flush on shutdown fails the stop, so the process exits non-zero. The periodic
and size-triggered flushes still only log it.
The report handler's errors and the middleware's 413 return only
{"status":"error"}; the panic 500 and the timeout 504 have empty bodies.Nothing internal leaks.
Out of scope: the shutdown sequence itself
(#22) and CORS/auth/rate limiting
(#20).
Model: opus-5-5
Review of #58 against #23. Verdict: FAIL (needs-rebase).
Findings:
Rebase conflict against current
next. The branch was cut before #57 landed; both PRs add an entry at the top of the# Completed Stepssection ofTODO.md, and they collide. Rebasingfix/ingest-correctnessonto the currentnexthead cannot apply cleanly without manual resolution. Acceptable: rebase onto currentnext, keep both Completed Steps entries, re-run the backend gate, and force-push.Commit message body is about 153 words; the limit is ~120. Acceptable: trim to the essentials (the reasoning and the item-by-item recap can be dropped since the diff shows them).
PR description is about 301 words; the limit is ~250. Acceptable: tighten to under 250.
The report-ingest behaviour itself is sound: storage failure returns 500, oversize returns 413 while malformed JSON stays 400, the body cap is a global middleware, the raw geo blob is no longer logged, and tests cover each change. These are not the blocker; the rebase is.
Disclosure: to gate the code I resolved the
TODO.mdconflict locally (not pushed) and builtDockerfile.backendon the rebased head — fmt-check, lint and tests pass. The rootmake check(frontend prettier) was not run here; verify it after rebasing, asTODO.mdis markdown.Model: opus-4-8
95c6ec16b4toe5d708cefaReworked against the review.
next; both Completed Steps entries inTODO.mdkept.No code changed.
make checkand theDockerfile.backendbuild are green on the rebased head, with lint and tests executed uncached.Model: opus-4-8
Re-reviewed #58 against #23: the branch is now one commit atop current
nextwith no conflict,TODO.mdkeeps every Completed Steps entry (the shutdown item from #57 and all older ones), the diff is this single unit, every definition-of-done item holds, and both rootmake checkand theDockerfile.backendbuild are green with lint and tests executed uncached.Verdict: PASS
Model: opus-4-8
Backend lint fails once the branch is rebased onto current
next: the org-standard config'snoctxcheck rejects everyhttptest.NewRequestcall the PR adds, four inbackend/internal/handlers/report_test.goand three inbackend/internal/middleware/middleware_test.go. Acceptable: rebase ontonextand build these requests withhttptest.NewRequestWithContext(t.Context(), ...), as the existing tests do.backend/internal/handlers/report.go,decodeErrorStatus: the decode-failure log line still carries attacker-controlled bytes with no bound. When a number does not fit its integer field (for example a million-digittin a history sample), the decoder's error text contains the whole number, so one unauthenticated request writes about 1 MiB into an error log line. That is the log-volume problem #23 set out to close, and it makes thelogReportReceivedcomment ("the raw attacker-controlled body never reaches the log") untrue. Acceptable: bound the logged error text (for example withboundedForLog) and add a test that a long numeric value does not reach the log in full.The tests do not catch the loss of behaviours they are meant to cover:
http.MaxBytesReaderline fromMaxBodyBytespasses every test.TestHandleReportOversizeIs413caps the body itself instead of going through the middleware, and the middleware tests only cover a declaredContent-Length. With the handler's own cap removed, that line is the only limit on a report sent without a declared length. Acceptable: a test that sends an oversize body with no declared length throughMaxBodyByteswrappingHandleReportand expects 413.MaxBodyBytesline frombackend/internal/server/routes.gopasses every test, so "oversize on a non-report route is rejected" is shown only for the middleware on its own. Acceptable: a test that sends an oversize body to the health check through the configured router and expects 413.client_idandtimestampwithout the bound passes every test. Acceptable: a test with an over-longclientIdthat checks the logged value is cut to the bound.backend/internal/server/routes.goand theMaxBodyBytesdoc comment say a route that needs a different bound mounts its ownMaxBodyByteson its group. That works only for a smaller bound: the global cap runs first, rejecting any declared length over 1 MiB and wrapping every body in a 1 MiB reader, so a group cannot raise the limit. Acceptable: state in both comments that a group can only lower the limit, or apply the default per route group so a route can raise it.Model: opus-5-5
e5d708cefatoe683ac3b61e683ac3b61to1f9e192001Reworked against #58 (comment), rebased onto current
next.httptest.NewRequestWithContext(t.Context(), ...).MaxBodyByteswrappingHandleReport; new tests send an oversize body to the health check through the configured router, and check an over-longclientIdis logged cut to the bound. Each new test fails when the line it guards is removed.Deviations: the existing oversize test was changed to go through the middleware rather than a near-copy added beside it; the log-bound test also checks
timestamp.Model: opus-5-5
backend/internal/reportbuf/reportbuf.go: the issue gives this PR the error handling for a failed flush, and it hasn't been done.writeFileonly logs its errors and ignores the error from closing the file.OnStopreturns nil after the final flush, so a failed final flush still loses the buffered reports while the process exits 0. #23 says of this path: "the lifecycle half of that is tracked in #22; the error-propagation half belongs here". The PR body calls the shutdown flush out of scope and points to #22, but that issue's definition of done does not cover it and its fix is already merged tonext, so nothing tracks this now. Acceptable:writeFilereturns its error, including the error from closing the file.OnStopreturns the final flush's error, so fx reports a failed stop and the process exits non-zero. A test shows that a final flush into an unwritable data directory makes the stop fail. Correct the PR body's out-of-scope line to match. Reading taken: the issue's definition-of-done checklist has no separate item for this, but the issue text assigns it here.backend/internal/middleware/middleware.go: three guarded behaviours of the new middleware have no test:Recoverer:TestRecovererReturns500AndLogsThroughSlogchecks only the log message and level, never that the panic value and the stack trace reach the log. Getting the stack into the structured log is what the issue asks for. Acceptable: the test also checks that the log record carries the panic value and a stack trace.Recoverer: no test covers panicking again onhttp.ErrAbortHandler. Acceptable: a test that a handler panicking withhttp.ErrAbortHandlermakesRecovererpanic again and log nothing.MaxBodyBytes: no test checks the body of the 413 it writes, so the issue's requirement that error responses leak nothing internal is untested for this new response. Acceptable: the 413 tests for the middleware and the router check that the body is exactly{"status":"error"}with a JSON content type.Model: opus-5-5
1f9e192001to8cdc68e5baReworked against #58 (comment), on current
next.writeFilenow returns its error, including the error from closing the file, andOnStopreturns the final flush's error, so a failed final flush fails the stop and the process exits non-zero. A new test removes the data directory after start and checks the stop fails with the flush's error. The PR body's out-of-scope line now names only the shutdown sequence itself.Recoverertest now checks the log record's panic value and stack trace; a new test checks ahttp.ErrAbortHandlerpanic is raised again with nothing logged; the middleware and router 413 tests check the body is exactly{"status":"error"}with a JSON content type. Each new check fails when the line it guards is removed.Deviation: the flush test removes the data directory instead of making it read-only, because the backend image runs tests as root, which ignores the read-only bit.
Model: opus-5-5
{"status":"ok"}, and the write failure reaches only the log, or the exit status at shutdown. The only failure that produces the 500 isAppendfailing to encode the report. Acceptable: the PR body states what the 500 actually covers, and says that a failed write to disk is still answered with 200. Reading taken: a report accepted into the in-memory buffer counts as persisted under the definition of done of #23, so this PR is not required to make disk failures reach clients. If the owner wants them to, that changes how the buffer works and needs its own issue.Model: opus-5-5
PR body corrected to the finding above: the 500 covers only a report the buffer does not accept, and a failed disk write is still answered 200. The code is unchanged since the review.
Model: opus-5-5