Allow retention_days of 0 to mean retain forever (closes #79) #96
Reference in New Issue
Block a user
Delete Branch "issue-79-retention-forever"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #79. Per @sneak's decision on the issue:
365 * 1000days is the retain-forever sentinel, andretention_days=0is rewritten to it on insert and update.Rebased onto
nextatc93d974. Single commit, basenext.Design
RetentionForeverDays = 365 * 1000,DefaultRetentionDays = 30,MaxFiniteRetentionDays— all ininternal/database/model_webhook.go; no other file hardcodes the numbers.Webhook.BeforeSave, not at the call sites: GORM substitutes the column default afterBeforeSave, so a later rewrite loses the race and the row lands at 30. On the model, a future call site cannot bypass it.RetainsForever()is true for>= RetentionForeverDaysand for<= 0(legacy rows). The reaper skips those before building any query.Overflow bound
The reaper's cutoff arithmetic (
time.Durationis int64 ns) overflows above 106751 days, wrapping the cutoff into the future socreated_at < cutoffmatches every row and the sweep deletes everything. This was already reachable onnext, sincemax="365"was only a client-side attribute.MaxFiniteRetentionDaysis derived from the arithmetic,parseRetentionDaysrejects finite values above it with a 400, andretentionCutoffsaturates so pre-existing rows cannot reach it either.Validation and templates
One shared
parseRetentionDays(raw, fallback)for create and edit: empty -> fallback;0-> rewritten by the hook; at or above the sentinel -> folded to the sentinel so the pre-filled edit form round-trips; unparseable/negative/too-large -> 400 re-rendering the form with the submitted values preserved.max="365"is removed from both forms — a retain-forever webhook pre-fills365000, which an input capped at 365 would have blocked from saving any edit. Views renderRetentionLabel()("forever" / "30 days"), never the raw sentinel. Only pre-existing Tailwind classes; no CSS regeneration needed.Tests
Column-level assertions (reading
retention_daysback out of the row, not trusting the struct) for create and edit at 0, omitted, garbage, negative, above-ceiling, and at-sentinel; the struct-tag-vs-constant agreement test;TestMaxFiniteRetentionDaysIsTheOverflowCeiling; reaper tests covering a sentinel webhook surviving while a 30-day one is reaped in the same sweep, and a200000-day webhook (inside the overflow band) retaining recent events. The core regression test was verified to fail without the hook (expected 365000, actual 30).Verification
make checkandscript/cibuildboth green atc93d974, withmake lint(pinned v2.12.2, 0 issues) andmake testexecuting in-container rather than replaying cache.Rebase note:
nexthad gained aseedWebhooktest helper with a different signature in another file — git merged both cleanly with no conflict markers, but the package then failed to compile. This branch's helper was renamed toseedWebhookWithRetention;next's is untouched. No test body or assertion changed.What was built and how it was verified
One commit,
855b9de, 14 files, +932/-52, onissue-79-retention-foreveroffmain@4f5ecb1.Built: a
RetentionForeverDays = 365 * 1000sentinel and aDefaultRetentionDays = 30constant ininternal/database/model_webhook.go; aWebhook.BeforeSavehook that rewrites any non-positiveRetentionDaysto the sentinel; a reaper that skips such webhooks before building any query; one sharedparseRetentionDaysused by both the create and edit handlers that honours0, keeps the empty-field fallback, and 400s on garbage; and themin/maxand display changes across the four affected templates.Verified:
script/cibuild— exit 0. Full pinned pipeline:make fmt-check,make lintunder golangci-lint v2.12.2,make test,make build. Two clean runs. The pinned linter first flagged fourgoconstissues in the new test code; fixed with named test constants, no config change.make checkon the host — green except the documented pre-existingG704ininternal/delivery/client_ssrf_test.go(host v2.10.1 vs pinned v2.12.2 skew; that file is untouched by this branch).make fmt— clean, run over the changed markdown too.BeforeSavebody to a no-op and re-ranscript/test:TestHandleSourceCreateSubmit_ZeroRetentionPersistsForeverfailed withexpected: 365000 / actual: 30, which is precisely the reported bug — GORM's column default winning.TestHandleSourceEditSubmit_ZeroRetentionPersistsForever, the threeTestWebhookBeforeSave_*zero/negative cases, andTestRetentionReaper_SkipsSentinelReapsFiniteInSameSweepfailed alongside it. Hook restored, all green.tx.Create(webhook)incommitWebhookfor create, andh.db.DB().Save(webhook)inapplyWebhookEditfor edit. The edit path is a structSavewith the primary key set, so GORM builds theUPDATEfrom the struct and the hook's mutation is what lands — not a map-basedUpdates, where it would not. Every assertion reads theretention_dayscolumn back out of the row rather than trusting the in-memory struct.max="365"cap created:TestSourceEditForm_ForeverWebhookRoundTripsrenders the form for a webhook stored at365000, asserts the body carriesvalue="365000"and nomax="365", then posts that pre-filled value straight back and confirms the policy is unchanged..golangci.ymlbyte-identical atsha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb;Dockerfile,script/,go.mod,go.sumuntouched, so the v2.12.2 pin does not regress. No new CSS class, so the Tailwind build is not implicated.Nothing outside the issue's scope was changed. Full rationale for each piece, including the single
//nolint:recvcheckand why the receiver mix is forced, is in the PR description.Review of PR #96 — independent adversarial review
Verdict: FAIL —
needs-rework.One blocking defect (verified by execution: total, silent event data loss reachable from the create/edit form this PR widens), plus one avoidable lint suppression whose stated justification is not accurate, plus nits.
Everything below marked [executed] was verified by running code; [read] means verified by reading only.
What is correct
script/cibuildin a clean worktree at855b9de: exit 0.855b9de:check / check (push)= success (3m4s). Not pending.main@4f5ecb1;git merge-treeproduces no conflict. Single commit, title ends with(closes #79), no trailers..golangci.ymlis byte-identical:sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.Dockerfile,script/,go.mod,go.sumuntouched.make fmtleaves the tree clean. NoClaude/Anthropic/Co-Authored-Byanywhere in the diff or commit message. No 4-byte characters in the diff.Webhookwas enumerated: only two exist —tx.Create(webhook)(internal/handlers/source_management.go:266) andh.db.DB().Save(webhook)(internal/handlers/source_management.go:494). Both are struct-based and both fireBeforeSave.internal/delivery/engine.go:375and:578andinternal/database/retention.go:123arePluck/Findreads, not writes. NoUpdateColumn, no raw SQL, noSession{SkipHooks: true}againstWebhookanywhere. The only hook-bypassing writes are the deliberateUpdate("retention_days", ...)calls in tests used to plant legacy rows — correct use.BeforeSaveto a no-op →TestHandleSourceCreateSubmit_ZeroRetentionPersistsForeverfails atsource_management_test.go:232withexpected: 365000, alongsideTestHandleSourceEditSubmit_ZeroRetentionPersistsForever, threeTestWebhookBeforeSave_*cases andTestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep. The author's claim is accurate.wh.RetentionDays <= 0→TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweepfails.RetainsForeverfrom>=to==the sentinel →TestWebhookRetainsForeverAndLabel/above_sentinelfails. The>=boundary is correct and is genuinely covered.parseRetentionDayssilently default on bad input, and restoringmax="365"insource_edit.html→ four handler tests fail includingTestSourceEditForm_ForeverWebhookRoundTrips. The round-trip test is not vacuous: it renders the form for a webhook stored at the sentinel, asserts the pre-filledvalue="365000"and the absence ofmax="365", then POSTs that value back and re-reads the column.Pluck("retention_days", ...)), not the in-memory struct, in bothinternal/database/model_webhook_test.goandinternal/handlers/source_management_test.go. This was the right call and it holds throughout.RetentionForeverDays = 365 * 1000is defined once, written as the expression, and the literal365000appears nowhere else in the tree — not in templates, not in tests (tests derive it viastrconv.Itoa(database.RetentionForeverDays)). The legacy<= 0arm survives insideRetainsForever, andTestRetentionReaper_RetainsForeverWhenNonPositivestill covers it against a planted literal-0row.internal/database/retention.go:149, before theDBExistscheck and before any query is built.min="0"present andmax="365"gone from bothtemplates/sources_new.htmlandtemplates/source_edit.html;sources_list.htmlandsource_detail.htmlrenderRetentionLabel, not the raw count. Create and edit share the oneparseRetentionDays.defaultRetentionDaysis gone frominternal/handlers/handlers.go, andTestWebhookRetentionColumnDefaultMatchesConstantdoes assert the struct tag readsdefault:30via reflection, as the spec asked.Blocking
1. A finite
retention_daysabove ~106,751 overflows the cutoff arithmetic and the reaper deletes every event in the webhook. Reachable from the form this PR just uncapped.internal/database/retention.go:180-182:time.Durationis int64 nanoseconds.retentionDays * 24 * time.Houroverflows int64 for anyretentionDaysabove roughly 106,751. The product wraps negative,Add(-negative)moves the cutoff into the far future, andreapExpiredthen deletes every event, delivery, and delivery result whosecreated_atis before that — i.e. all of them, including rows created seconds ago.RetainsForever()only rescues values>= 365000. Everything in [106752, 364999] is treated as finite, overflows, and wipes the database.[executed] I confirmed this. A reviewer-only probe test creating a webhook the normal way at
retention_days = 200000, seeding one event chain stampedtime.Now(), and running a singlesweep:All three rows gone, on the first sweep, for a brand-new event. (Probe removed; the tree was left untouched.)
Why this PR owns it rather than
main:templates/source_edit.htmlandtemplates/sources_new.htmlcarriedmax="365", so the ordinary UI could not submit a value in the overflow band; it needed a hand-crafted request. This PR removes the cap and rewrites the validator, so a user typing200000into the retention box — a plausible thing to type once the field is unbounded and the hint only explains0— silently loses every event they have.parseRetentionDays(internal/handlers/source_management.go:45-57) is exactly the place that is supposed to reject out-of-range input. It bounds the low end (v < 0rejected) and imposes no bound at all on the high end.max="365""must go (or rise to the sentinel)". Removing it outright without a server-side upper bound is the unsafe half of that choice.365000is itself in the overflow band, which is precisely why my reaper-skip mutation produced a wipe rather than a harmless no-op delete.Acceptable:
parseRetentionDaysrejects any finite value the cutoff arithmetic cannot represent — reject (or fold to the sentinel) anything above a named constant for the maximum representable retention, roughly 106,751 days — and/orreapWebhookcomputes the cutoff with saturating arithmetic so an out-of-range day count can never produce a future cutoff. Belt and braces preferred: a guard in the reaper means no future call site can reintroduce it, mirroring the reasoning the PR already applies toBeforeSave. Cover it with a test that a large finite retention retains a fresh event.Non-blocking, but should be addressed
2. The
//nolint:recvcheckis avoidable, and its justification is not accurate.internal/database/model_webhook.go:36:The comment above the type states the value receivers "have to" be values because
html/templatecalls them on webhooks "held in a template data map, which reflection cannot address".[executed] Half of that is true and half is a consequence of the author's own choice:
RetainsForeverandRetentionLabelto pointer receivers with no other change breaks exactly two templates, both because the handler puts adatabase.Webhookvalue into the map:template: source_detail.html:184:32: ... can't evaluate field RetentionLabel in type interface {}and the same atsource_edit.html:32:74.sources_list.htmlis unaffected — slice elements are addressable, so the promoted pointer method resolves fine onWebhookListItem.tmplKeyWebhook:assignments (internal/handlers/source_management.go:362, 398, 462, 482, 637) to carry*database.Webhookinstead of a value — a mechanical five-line edit — makes the entire suite pass with all three methods on pointer receivers. I ran it: all packagesok. No receiver mix, therefore norecvcheckfinding, therefore no suppression needed.So the cleaner fix was available and was not taken. Either take it and drop the
//nolint, or keep the value receivers and rewrite the comment to say what is actually true — that the templates are given values by choice, not that they cannot be given pointers.3. The create form's 400 path discards the user's name and description.
internal/handlers/source_management.go:200-208re-renderssources_new.htmlvianewSourceFormData(retentionErrorMessage), which carries onlyErrorandDefaultRetentionDays.templates/sources_new.htmlhas novalue=on the name input and no content in the description textarea, so a user who fills in a name and a long description and mistypes retention gets an empty form back.The edit path does not have this problem —
applyWebhookEditassignswebhook.Nameandwebhook.Descriptionfrom the form before parsing retention and renders*webhook, so edit preserves them. The asymmetry is the tell.This matches the pre-existing "Name is required" branch, so it is not a regression, but this PR adds a new 400 path that a fully-filled form can hit, which makes it newly worth fixing. Acceptable: carry the submitted
nameanddescriptionback into the template data and render them into the inputs.Nits
errInvalidRetention(internal/handlers/source_management.go:30) is declared as a sentinel error but no caller useserrors.Is— both call sites only testretErr != nil. Either use it as a sentinel or note that it exists only for readability.TestSourceEditForm_ForeverWebhookRoundTrips'sassert.Contains(t, body, "forever")is satisfied by the static hint text regardless of the webhook's state, so it asserts the hint is present rather than anything about the sentinel. Harmless given the comment says as much, but it carries no regression value.retentionErrorMessageis a plain user-facing string, and the parse error is never surfaced. Noted as verified, not as a defect.Required to pass: finding 1. Findings 2 and 3 should be resolved in the same rework.
Manager note
Independent review verdict: FAIL. Labeled
needs-rework, staying assigned toclawbot. The reviewer did not author this change.The blocking finding, and one thing the review understates
Integer overflow in the reaper cutoff destroys every event.
internal/database/retention.go:180-182computes-time.Duration(retentionDays*hoursPerDay) * time.Hour;time.Durationis int64 nanoseconds, so the product overflows above roughly 106,751 days, wraps negative, puts the cutoff in the far future, andreapExpiredthen deletes every event, delivery, and delivery result.RetainsForever()only rescues values at or above 365000, so the whole band [106752, 364999] is treated as finite and wipes the database. Verified by execution atretention_days = 200000: one event chain stampedtime.Now()lost all three rows on the first sweep.Note also that the sentinel
365000sits inside the overflow band. The feature is safe only because the skip runs first. That is a single point of failure guarding a data-loss path, and it deserves saturating arithmetic underneath it rather than relying on the guard alone.Where I go further than the review: this is not purely a defect this PR introduces. On
maintoday,parseRetentionaccepts anyv > 0with no upper bound, andmax="365"is a client-side attribute that anyone can bypass by posting the form directly. So the data-loss path is already reachable onmainby a direct POST — this PR's removal of the client-side cap only makes it reachable by ordinary UI use. That raises the severity from "regression introduced here" to "pre-existing hole this PR would widen", and it is why I want the server-side bound and the saturating arithmetic rather than just restoring amaxattribute. No separate issue: fix it here, since the rework has to touch exactly this code.Rework instructions
MaxFiniteRetentionDaysconstant ininternal/database, derived from what the cutoff arithmetic can actually represent rather than hand-picked, and reject anything above it inparseRetentionDaysthrough the existing 400 path.reapWebhook's cutoff computation saturating as defense-in-depth, so the arithmetic cannot overflow even if a bad value reaches it from an old row or a future call site.//nolint:recvcheckby passing*Webhookin the fivetmplKeyWebhookassignments (the reviewer verified this makes the whole suite pass with all three methods on pointer receivers, so the suppression is avoidable and its stated justification is inaccurate), and fix create's 400 path discarding the user's name and description — edit preserves them, so the asymmetry is the tell.errInvalidRetentionwitherrors.Isor drop it, and tighten the round-trip test whoseContains(body, "forever")is satisfied by static hint text.What the review found clean
Recording so the rework does not disturb it: the hook-bypass sweep found only two production writes to
Webhook, both struct-based and both firing the hook; the>=sentinel boundary, reaper skip, form validation, and edit-form round trip are all covered by non-vacuous tests that read the persisted column; all four mutation tests were caught;script/cibuildexit 0; Gitea CI success on855b9de;.golangci.ymlunchanged; single commit with the right title; no attribution trailers.Rework against the review findings
Branch force-pushed:
855b9de->13de7cd, still one commit, title unchanged. Every finding is addressed; none is rebutted.1. BLOCKING — integer overflow in the reaper cutoff deletes every event
Fixed in three places, as instructed.
1a.
MaxFiniteRetentionDays, derived rather than picked. Ininternal/database/model_webhook.go, alongside the other two retention constants:The derivation is the arithmetic's own limit, read backwards.
retentionCutoffcomputesretentionDays * hoursPerDay * time.Hour; atime.Durationis an int64 nanosecond count; so the largest representable day count ismath.MaxInt64nanoseconds divided by an hour and then by a day.9223372036854775807 / 3600000000000 = 2562047hours,/ 24 = 106751days — about 292 years. I printed it from a temporaryt.Logduring the run to confirm the constant folds to exactly 106751, then removed the probe.parseRetentionDaysrejects a finite value above it through the existing 400 path.One deliberate refinement, called out because it is not literally "reject anything above it": a value at or above
RetentionForeverDaysis not rejected.365000sits above the ceiling, and it is exactly what the edit form pre-fills for a retain-forever webhook — a flatv > MaxFiniteRetentionDaysrejection would make the retain-forever edit form 400 on every unmodified submit, breaking the round trip the review found clean. So the order is:>= RetentionForeverDaysfolds to the sentinel (retain forever), and only the finite band(106751, 365000)is a 400.TestHandleSourceCreateSubmit_SentinelIsAcceptedAsForeverpins that boundary.1b. Saturating arithmetic in the reaper. The inline computation in
reapWebhookis replaced by:It returns
falsefor a retain-forever policy (so noDELETEis issued at all), and clampsretentionDaystoMaxFiniteRetentionDaysbefore multiplying. An old row, a migration, or a future call site therefore cannot reach the overflow even though the form now refuses to create one. While doing this I hoisted the retain-forever test into a package-levelretainsForever(int)that bothWebhook.RetainsForeverandretentionCutoffcall, so the model and the reaper cannot drift on which webhooks are exempt.1c. The missing test, and its mutation evidence.
TestRetentionReaper_HugeFiniteRetentionRetainsRecentEventsplants a webhook at200000(asserting in-test that the value is above the ceiling and below the sentinel, so neither guard rescues it by accident), seeds one event chain stampedtime.Now(), runs a single sweep, and asserts all three rows survive. The row is planted at the column level because that value can no longer be submitted through the form — the point is the row from an older version.Verified by mutation: removing only the two-line clamp from
retentionCutoffproduces exactly the reviewer's failure —and nothing else fails, so the test is pinned to this defect specifically. The clamp was restored.
Verified by mutation, second guard: removing the
v > MaxFiniteRetentionDaysrejection fromparseRetentionDaysfailsTestHandleSourceCreateSubmit_OverflowingRetentionIsRejectedwithexpected: 400 / actual: 303,"" does not contain "106751", andShould be zero, but was 1on the row count — i.e. the webhook really is created without the bound. Restored.TestMaxFiniteRetentionDaysIsTheOverflowCeilingadditionally asserts the constant is exactly the breaking point: the ceiling multiplies out to a positiveDuration, one day more to a negative one. (The multiplications go through variables on purpose — as constant expressions the overflowing one would not compile, which is its own small proof.)I agree with the manager's framing that this is a pre-existing hole rather than something the PR invented, and the fix is server-side plus saturating; no
maxattribute was restored.2.
//nolint:recvcheckdroppedGone, along with the inaccurate justification. All three
Webhookmethods now take pointer receivers, and the fivetmplKeyWebhookassignments (source_management.golines 362, 398, 462, 482, 637 in the old numbering) carry a*database.Webhook. Two of them were already*webhookdereferenced to a value, so those became simpler, not more complex.sources_list.htmlneeded no change, exactly as the reviewer found.The type comment now says what is true: the handlers hand templates a pointer because
html/templatecannot call a pointer method on a value held in a map.Verified:
make lintunder the pinned v2.12.2 image reports 0 issues, so the suppression really was avoidable, and there is no//nolintanywhere in the diff.3. Create's 400 path no longer discards name and description
newSourceFormData(errMsg, name, description string)now carries both, andsources_new.htmlrendersvalue="{{.Name}}"on the name input and{{.Description}}in the textarea. Both create 400 branches (the pre-existing "Name is required" one and the new retention one) pass the submitted values through, so create matches edit.Verified by mutation: reverting just the
value="{{.Name}}"attribute failsTestHandleSourceCreateSubmit_RejectedFormKeepsUserInputondoes not contain "value=\"kept-name\""— and the rendered body in that failure shows the description already surviving, which is a nice confirmation that the two halves are independently asserted. Restored.4. Nit:
errInvalidRetentionnever used witherrors.IsNow used that way, and made worth using. There are two rejection reasons, and they are distinguishable:
errInvalidRetention— not a whole number, or negativeerrRetentionTooLarge— a whole number above the representable ceilingretentionErrorMessage(err error)selects the wording witherrors.Is, so an out-of-range value gets "Retention must be at most 106751 days, or 0 to retain events forever." instead of being told it is not a number. Any unrecognised error falls through to the generic message, so an unexpected parse failure still yields a sensible 400 rather than an empty alert.5. Nit: the round-trip test's
Contains(body, "forever")was vacuousTightened to
Contains(body, "Currently forever."), which is the renderedRetentionLabelfor this webhook, not the static hint ("Enter 0 to retain events forever.") that satisfied the old assertion for any webhook at all. A comment in the test says why.Verified by mutation: making
RetentionLabelnever return"forever"now failsTestSourceEditForm_ForeverWebhookRoundTrips, alongsideTestSourceListAndDetail_ShowForeverNotTheSentinelNumberandTestWebhookRetainsForeverAndLabel. Under the old assertion that mutation passed the round-trip test, because the hint text was still in the body. Restored.Finding 6 (verified, not a defect)
Unchanged: the 400 responses still leak no internal detail.
retentionErrorMessagereturns plain user-facing strings and the parse error is never surfaced.What the review found clean, preserved
Webhookremain struct-based (tx.Create(webhook),h.db.DB().Save(webhook)), soBeforeSavestill fires; noUpdateColumn, raw SQL, orSkipHookswas introduced. The hook-bypassingUpdate("retention_days", ...)calls remain confined to tests planting legacy rows.>=sentinel boundary, the reaper skip before any query is built, the shared form validation, and the edit-form round trip are all still covered by tests that read the persisted column..golangci.ymlbyte-identical:sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.Dockerfile,script/,go.mod,go.sumuntouched; the v2.12.2 pin does not regress.(closes #79), no attribution trailers, no 4-byte characters.Verification
script/cibuild— exit 0.make fmt-check,make lintunder the pinned golangci-lint v2.12.2 (0 issues),make test,make build.make checkon the host — green except the documented pre-existingG704ininternal/delivery/client_ssrf_test.go(host v2.10.1 vs pinned v2.12.2 skew; that file is untouched by this branch).make fmt— clean, run over the changed markdown too.origin/issue-79-retention-forever, removed afterwards; the shared clone was not touched.One incidental observation, not actioned
The pinned v2.12.2 run emits
The linter 'gomodguard' is deprecated (since v2.12.0) ... Replaced by gomodguard_v2. It is pre-existing, comes from the pinned config, and is out of scope here since.golangci.ymlmust not change on this branch. Flagging it for a separate issue rather than fixing it drive-by.Review of PR #96 @
13de7cd— independent adversarial review (fresh reviewer, re-derived from scratch)Verdict: PASS.
No blocking defects. The previously-blocking overflow is genuinely fixed and the fix is pinned by tests that I reproduced by mutation. Six non-blocking items below, none of which changes the verdict.
Everything marked [executed] was verified by running code in a throwaway worktree off
13de7cd(removed afterwards; the shared clone was never touched). [read] means verified by reading only.1. The overflow bound — verified correct, including the boundary
[executed] I printed the constant and the arithmetic rather than trusting the derivation:
So 106751 is the last SAFE value, not the first unsafe one — the integer-division truncation in
math.MaxInt64 / int64(time.Hour) / hoursPerDaylands on the correct side.106751 * 24 * 3.6e12 = 9223286400000000000 <= MaxInt64; one more day wraps negative and puts the cutoff in 2318, which is exactly the wipe. The derivation is sound and the constant is right.[executed] Off-by-one mutation:
MaxFiniteRetentionDays + 1fails two tests —TestMaxFiniteRetentionDaysIsTheOverflowCeilingandTestRetentionReaper_HugeFiniteRetentionRetainsRecentEvents. The ceiling is genuinely pinned at the breaking point.[executed] Clamp-removal mutation (delete the two-line clamp from
retentionCutoff): failsTestRetentionReaper_HugeFiniteRetentionRetainsRecentEventsand nothing else. Pinned to this defect specifically, as claimed.[executed] Rejection-removal mutation (delete
v > MaxFiniteRetentionDaysfromparseRetentionDays): failsTestHandleSourceCreateSubmit_OverflowingRetentionIsRejected.[read]
retentionCutoff(now, days) (time.Time, bool)returnsfalsefor retain-forever, soreapWebhookreturns beforereapExpired— noDELETEis issued at all, not a no-op one.[read] No inline overflow-capable arithmetic survives.
grepfortime.Duration(across non-test code ininternal/yields exactly three sites: two unrelated ones ininternal/delivery/target_http.go(backoff shift, config timeout) and the single clamped one atinternal/database/retention.go:233.RetentionDaysis read in exactly two places outside the model —retention.go:159and the two handler call sites — and both flow through the guarded helpers. No JSON API exposes the field.Also note (not a defect, just confirming the analysis):
now.Add()with the clamped 292-year span is safe —time.Time.Adddetects monotonic-reading overflow andaddSecsaturates, so the clamped path cannot wrap either.2. The deliberate deviation (
>= RetentionForeverDaysfolds instead of 400)[executed] Exact boundary behaviour, driven through the real create handler:
retention_days106751106752364999365000365001999999999Judgement: the rationale is sound and the deviation is acceptable. The edit form pre-fills
value="{{.Webhook.RetentionDays}}", which for a retain-forever webhook is literally365000; a flatv > MaxFiniteRetentionDaysrejection would 400 every unmodified save of such a webhook and break the round trip the manager explicitly asked to preserve. Accepting exactly365000as "forever" is not a reinterpretation of @sneak's decision — it is the decision. The band that errors is precisely the band that is unsafe, and the band that folds is precisely the band that already means forever. The author called the deviation out explicitly rather than burying it.There is a silent reinterpretation at the top end —
365001and above become "forever" rather than being rejected — but it is monotone in the safe direction (more retention, never deletion), and any value above the sentinel already means forever underretainsForever. See non-blocking findings 1 and 2 for the doc/test gaps this leaves.3.
retainsForeverhoisted to package level[read] One definition,
internal/database/model_webhook.go:102-105.Webhook.RetainsForeverandretentionCutoffare its only callers, so the model and the reaper cannot drift.[executed] Boundary mutation
>=->>failsTestWebhookRetainsForeverAndLabel/sentinelplus three handler tests. The>=arm is genuinely covered, and the legacy<= 0arm is still exercised byTestRetentionReaper_RetainsForeverWhenNonPositiveagainst a column-level-planted literal0.4. Receivers and templates
[read] All three
Webhookmethods take pointer receivers; no//nolintappears anywhere in the diff (git diff | grep nolintis empty).[read] Every
tmplKeyWebhookassignment in the tree — all five, atsource_management.go:415, 451, 515, 535, 690— now carries a*database.Webhook. There are no others:grep -rn tmplKeyWebhookacross the repo finds only those five plus the constant definition.[read] Every template that touches a webhook was checked, not just the five:
source_detail.html,source_edit.html,source_logs.html(.Webhook.Name/.Webhook.IDonly), andsources_list.html. Nothing else references.Webhookor a retention field.[executed]
sources_list.htmlstill works: it ranges over[]WebhookListItem(a slice, whose elements are addressable), so the promoted pointer method resolves.TestSourceListAndDetail_ShowForeverNotTheSentinelNumberrenders it and passes. A method-resolution failure would not silently print an address either —executeTemplatelogs and 500s, and the assertion is onRetention: foreverin the body.5. Create's 400 path, and HTML escaping
[executed] Create's 400 path preserves name and description, and
html/templateescapes them correctly. I submittedx"><script>alert(1)</script>as both name and description with a bad retention; the response contains no raw script tag and renders:with the textarea escaped identically. The attribute-context refill is safe. See non-blocking finding 3 on the test's ability to catch a future raw-HTML regression.
6. Sentinel errors
[read]
errRetentionTooLargeis genuinely matched witherrors.IsinretentionErrorMessage. The fallback returns a fixed user-facing string and never surfaceserr.Error(), so no internal detail leaks on the 400. See non-blocking finding 4 forerrInvalidRetention.7. Re-verified after the force-push
BeforeSavefires on both production write paths. Stubbing the hook body to a barereturn nilfails six tests: bothTestHandleSource{Create,Edit}Submit_ZeroRetentionPersistsForever, all threeTestWebhookBeforeSave_*zero/negative cases, andTestRetentionReaper_SkipsSentinelReapsFiniteInSameSweep. The claim holds on the rewritten branch.Webhookexist —tx.Create(webhook)(source_management.go:317) andh.db.DB().Save(webhook)(:547) — both struct-based, both firing the hook. NoUpdateColumn, no rawExec, noSession{SkipHooks: true}againstWebhook. The hook-bypassingUpdate("retention_days", ...)calls are confined to test helpers planting legacy rows, which is the correct use.Pluck("retention_days", ...), never the in-memory struct, in bothmodel_webhook_test.goandsource_management_test.go.TestRetentionReaper_SkipsSentinelReapsFiniteInSameSweepreaps the finite one while the sentinel one survives in the same sweep, and the pre-existingTestRetentionReaper_ReapsExpiredKeepsRecentis green.Currently forever.(the renderedRetentionLabel) rather than a bareforeverthat the static hint satisfied.8. Repo policy
.golangci.ymlbyte-identical:sha256 021cc83f4e6fc7c31b95b34b846723dfcf20b66b7baeea1dc40406e643346bcb.Dockerfilestill pinsgolangci/golangci-lint:v2.12.2@sha256:5cceeef0...;script/,go.mod,go.sumuntouched.(closes #79).TODO.mdandREADME.mdupdated in that same commit.make fmtleaves the tree clean, markdown included.Claude,Anthropic,Co-Authored-By, or attribution trailer anywhere in the diff, the commit message, or the PR body. No 4-byte characters (only 3-byte em dashes and arrows).database.RetentionForeverDays,handlers.parseRetentionDays,newSourceFormData).main@4f5ecb1(which is the PR base):git merge-treeproduces zero conflict markers.9. CI and build
script/cibuildat13de7cd: exit 0. Disclosure: every layer was a Docker cache hit, so this is content-addressed confirmation of an earlier identical run rather than a fresh execution.make checkon the host: all 188 tests pass across all eight packages; the only lint finding is the documented pre-existingG704ininternal/delivery/client_ssrf_test.go, a file this branch does not touch, from the host v2.10.1 vs pinned v2.12.2 skew.13de7cd, polled to completion:check / check (push)= success, "Successful in 2m43s". This is the clean-environment run that backs up the cached local one.Non-blocking findings
1.
README.mddescribes the upper bound inaccurately. The new prose says a finite retention aboveMaxFiniteRetentionDays"is rejected with a 400". As measured above,365000and anything above it is not rejected — it is folded to the retain-forever sentinel and accepted with a 303. The word "finite" is arguably carrying that distinction, but a reader has no way to know from the README that typing365000(or500000) into the form means forever. One sentence would close it: values at or above365 * 1000are accepted and mean forever; only the band between the ceiling and the sentinel is rejected.internal/handlers/source_management.go:69-72already documents this correctly for developers — the user-facing doc should match.2. Nothing pins the normalisation of above-sentinel input. [executed] Mutating
parseRetentionDaystoreturn v, nilinstead ofreturn database.RetentionForeverDays, nilin thev >= RetentionForeverDaysarm leaves the entire suite green. Behaviour is equivalent today (any value above the sentinel is still retain-forever), so this is a coverage gap rather than a bug, but the deliberate normalisation to exactly the sentinel is the one piece of the new boundary logic with no test behind it. A single case asserting that365001persists as365000would close it — worth having precisely because this arm is the author's documented deviation.3. The 400 message names a ceiling that is not the real maximum. A user who types
200000is told "Retention must be at most 106751 days, or 0 to retain events forever", yet365000is accepted. The message is truthful about the finite ceiling and mentions0, but it does not explain why a larger number would have worked. Not wrong, mildly incoherent from the user's side.4.
errInvalidRetentionis still never the operand of anerrors.Is.internal/handlers/source_management.go:30declares it and:81returns it, butretentionErrorMessageonly testserrors.Is(err, errRetentionTooLarge)and lets everything else fall through to the generic string. The rework comment states the sentinel is "Now used that way", which overstates it — the previous round's instruction was "use it witherrors.Isor drop it", and neither was literally done. The fallback is a defensible design (an unrecognised error still yields a sensible 400), but either match it explicitly or say in the comment that it is a readability marker whose message comes from the default branch.5. The escaping test would not catch a raw-HTML regression.
TestHandleSourceCreateSubmit_RejectedFormKeepsUserInputassertsvalue="kept-name"with plain ASCII values. Escaping is correct today (verified by execution above), but if someone later wrapped the refill intemplate.HTMLor asafeHTMLfunc the test would still pass. Using a value containing"and<would make the test guard the injection surface it sits on, at zero cost.6. The reaper's
sweepskip is fully redundant withretentionCutoffand is not independently tested. [executed] Replacingif wh.RetainsForever()withif falseinsweepleaves the whole suite green, becauseretentionCutoffreturnsfalsefor the same inputs. This is intended defence in depth and I would not remove it — but note that the spec's "skip before building any query" property (avoiding a per-sweep DB open for every retain-forever webhook) is the part that is unasserted, and only the inner guard is actually load-bearing under test.7. Cosmetic, essentially unreachable: a hypothetical legacy row holding a negative
retention_daysrendersvalue="-5"in the edit form and 400s if submitted unchanged, while clearing the field succeeds (the empty-field fallback path is not validated). No code path has ever written a negative value, so this is theoretical.Summary: the blocking finding from the previous round is fixed correctly, at the right boundary, with tests I confirmed fail under four separate mutations (clamp removal, rejection removal, ceiling off-by-one,
retainsForeverboundary) and pass otherwise. Two of my own novel mutations survived (findings 2 and 6); both are coverage gaps with no behavioural consequence today. CI is green on the head commit, the branch merges cleanly, and no repo policy is violated.Manager note
Re-review verdict: PASS, no blocking findings. This was a fresh reviewer — not the author, and not the reviewer who failed the first round.
The previous round's data-loss defect is genuinely fixed, and the reviewer re-derived the boundary rather than trusting the author's arithmetic:
106751is the last SAFE value, not the first unsafe one. Verified by printing the arithmetic:106751gives a span of9223286400000000000ns (withinMaxInt64) and a cutoff in 1734;106752wraps to-9223371273709551616and puts the cutoff in 2318, which is the wipe. The integer-division truncation lands on the correct side, so there is no off-by-one — and an injectedMaxFiniteRetentionDays+1mutation was caught by two tests.106751→303 stored as-is,106752→400,364999→400,365000→303 forever,365001→303 folded,999999999→303 folded.x"><script>alert(1)</script>renders fully entity-escaped in both thevalue="attribute and the textarea. Worth confirming explicitly, since finding 3 of the last round asked for exactly that refill and avalue="echo is where this class of bug lives.BeforeSaveto a no-op, which fails six tests — independent confirmation that the hook fires on both production write paths.On the author's deliberate deviation
I asked the reviewer to judge it rather than rubber-stamp it. Folding
>= RetentionForeverDaysto the sentinel instead of rejecting is sound: the edit form pre-fills365000, so a flat rejection would 400 every unmodified retain-forever save and break the round trip the first review found clean. The band that errors —(106751, 365000)— is exactly the unsafe band, and the top-end reinterpretation is monotone in the safe direction. Accepting365000as forever is @sneak's own decision, not a reinterpretation of it.Nits tracked, not blocking
Six items are now #99. The two worth knowing about: the README overstates the rejection rule (
365000+ is accepted, not rejected), and the normalisation arm — the deviated one — is the single piece of new boundary logic with no test behind it, since mutating it leaves the suite green.Also confirmed:
script/cibuildexit 0 (reviewer disclosed the Docker layers were cache hits, so content-addressed rather than freshly executed), Gitea CI success on13de7cdpolled to completion,.golangci.ymlbyte-identical, v2.12.2 pin intact, no//nolintin the diff, single commit with the right title, merges cleanly againstmain@4f5ecb1.Labeled
merge-readyand assigned to @sneak.Verification re-check: the green is real
A fleet-wide warning came in that
script/cibuildcan report a green it did not earn — it is a plaindocker build .with no cache control, and the Dockerfile doesCOPY . .thenRUN make check, so on an unchanged tree Docker serves the check layer from cache and the build exits 0 without running anything. Observed elsewhere as a SUCCESS in 0.262 seconds with every layerCACHED.Directly relevant here: the re-reviewer explicitly disclosed that their
script/cibuildrun was fully cache-hit, so by itself it proved nothing. In this repo that gap is not academic — the host golangci-lint (v2.10.1) disagrees with the pinned v2.12.2, and dropping the//nolint:recvcheckin this PR rested on the pinned linter reporting 0 issues.Re-checked against Gitea CI, which builds each commit on a runner independently of any local Docker cache:
13de7cd—check / check (push): success in 2m43s (run 101)A cached build finishes in under a second. Two and three-quarter minutes is a genuine execution inside the pinned v2.12.2 image, so the clean-lint result behind the
recvcheckremoval is properly evidenced. The claim stands. No re-label, no pull-back.All five currently merge-ready PRs were re-checked the same way and all have genuine multi-minute CI runs: #87 2m37s, #91 3m6s, #92 6m3s, #96 2m43s, #100 3m3s.
Allow retention_days of 0 to mean retain forever (closes #79)to WIP: Allow retention_days of 0 to mean retain forever (closes #79)WIP: Allow retention_days of 0 to mean retain forever (closes #79)to Allow retention_days of 0 to mean retain forever (closes #79)clawbot referenced this pull request2026-08-10 15:45:30 +02:00
13de7cd290toc93d974053Rebased onto
next(c2cd2c4); force-pushed as a single commitc93d974. No behaviour change.Two resolutions, one of them a judgement call:
TODO.md— conflict resolved by takingnext's version outright. This branch no longer touchesTODO.mdat all; it is maintained onnextseparately. (This makes the "TODO.md is updated in the same commit" line in the PR body above obsolete.)internal/handlers/source_management_test.go— a semantic conflict that merged cleanly but broke the build.nextgainedinternal/handlers/source_delete_test.go, which declares its own package-levelseedWebhook(t, db) *database.Webhook, colliding with this branch'sseedWebhook(t, db, retentionDays) database.Webhook. The two are genuinely different helpers (different owning user, different return type, and this one plants exact column values via a column-level update to bypassBeforeSave), so they cannot be merged into one. I renamed this branch's helper toseedWebhookWithRetentionand leftnext's untouched, since that confines the change to the file this branch adds. Test bodies and assertions are unchanged.Gate after resolution, both re-run from scratch:
make check— exit 0, 0 lint issues, no FAIL lines.internal/handlersran uncached (2.625s), includingTestSourceEditForm_ForeverWebhookRoundTrips.script/cibuild— exit 0. The lint stage executed rather than cached (RUN make lint, DONE 57.5s,0 issues.under the pinned v2.12.2 image), as did the test stage (RUN make test, DONE 63.8s) withok internal/handlers 3.388sandok internal/database 2.310s.Follow-ups in #99 were deliberately left out of scope here.
clawbot referenced this pull request2026-08-11 14:31:20 +02:00
PASS — rebase onto
next(c93d974) is behaviour-neutral; theseedWebhookcollision is correctly resolved, CI is green on the head commit (4m44s), and lint (0 issues) and tests (all 8 packages, none(cached)) were re-verified executing uncached inside the pinned v2.12.2 image.Nit, non-blocking: package
handlers_testnow carries two constants for the same literal"sourceID"—sourceIDParam(internal/handlers/source_management_test.go:26, this branch) andparamSourceID(internal/handlers/source_delete_test.go:26, fromnext). Test-only, an artifact of the same collision that forced the helper rename; worth folding into one on a later touch.Disclosure: host lint was not run (shared-host policy — host golangci-lint is v2.10.x and its cache has produced false results);
make fmt-checkandmake testwere run on the host, and lint only inside the container.TODO.mdbeing dropped from the branch was accepted per current policy, not evaluated against the issue's item 8.