RetentionDays cannot be set to 0 (retain forever) via the normal create path #79

New Issue

clawbot · 2026-08-07T15:26:51+02:00

clawbot commented

2026-08-07 15:26:51 +02:00

Surfaced during #63 / PR #78.

Webhook.RetentionDays carries gorm:"default:30", so GORM substitutes the column default (30) for a zero value on Create. The retention reaper correctly treats a RetentionDays of 0 or less as "retain forever", but that value is unreachable through the normal create path: a webhook created without an explicit positive value gets 30, and there is no way to express "keep forever" from the create/edit flow.

Net effect today: every webhook has an effective minimum retention (default 30 days) and cannot be set to keep events indefinitely.

Definition of done:

a user can configure a webhook to retain events forever (options: a distinct sentinel value, a nullable column, or an explicit "never expire" affordance that persists a non-positive value past the GORM default)
the reaper's retain-forever branch is reachable end to end from a normal create/edit
covered by a test

Note: this ties into the web-UI cleanup (#57) — whether and how retention is surfaced in the UI.

Surfaced during #63 / PR #78. `Webhook.RetentionDays` carries `gorm:"default:30"`, so GORM substitutes the column default (30) for a zero value on `Create`. The retention reaper correctly treats a `RetentionDays` of 0 or less as "retain forever", but that value is unreachable through the normal create path: a webhook created without an explicit positive value gets 30, and there is no way to express "keep forever" from the create/edit flow. Net effect today: every webhook has an effective minimum retention (default 30 days) and cannot be set to keep events indefinitely. Definition of done: - a user can configure a webhook to retain events forever (options: a distinct sentinel value, a nullable column, or an explicit "never expire" affordance that persists a non-positive value past the GORM default) - the reaper's retain-forever branch is reachable end to end from a normal create/edit - covered by a test Note: this ties into the web-UI cleanup (#57) — whether and how retention is surfaced in the UI.

sneak commented

2026-08-07 15:32:10 +02:00

let's use 365*1000 days as the retain-forever value.

sneak commented

2026-08-07 15:32:54 +02:00

attempting to set retentiondays to 0 should rewrite this on insert/update to be the 365*1000 value in the db

clawbot referenced this issue

2026-08-07 18:07:44 +02:00

1.0/mvp #33

clawbot commented

2026-08-09 04:09:59 +02:00

Implementation requirements

Baseline: main @ 4f5ecb1. @sneak's decision in the comments above is the design and is not up for reinterpretation: 365 * 1000 days is the retain-forever value, and an attempt to set retention_days to 0 is rewritten to that value on insert and update.

Current state, confirmed by reading

internal/database/model_webhook.go:12 — RetentionDays int \gorm:"default:30" json:"retentionDays"`. The GORM column default is why a zero value becomes 30 on Create`.
internal/handlers/handlers.go:30 — defaultRetentionDays = 30, a second source of truth for the same 30.
internal/handlers/source_management.go:145-185 — the create path; internal/handlers/source_management.go:445-461 — parseRetention, the edit path.
internal/database/retention.go:143 — if wh.RetentionDays <= 0 { continue }, the current retain-forever branch.
Templates: templates/sources_new.html:31 and templates/source_edit.html:31 both use min="1" max="365"; templates/sources_list.html:28 renders {{.RetentionDays}}d retention; templates/source_detail.html:184 renders Retention: {{.Webhook.RetentionDays}} days.

1. The sentinel

Add one exported constant in internal/database — e.g. RetentionForeverDays = 365 * 1000 — written as that expression, not as a bare 365000, so its meaning is self-evident. It is the single source of truth; nothing else may hardcode the number.

2. Rewrite on insert and update

Implement the rewrite as a GORM BeforeSave hook on Webhook so it fires on create and update and cannot be bypassed by a future call site (a REST API is on the roadmap). Any RetentionDays <= 0 becomes RetentionForeverDays.

Note the interaction that makes the hook necessary rather than optional: the gorm:"default:30" tag substitutes 30 for a zero value at insert time, so a rewrite that happens anywhere later than the hook loses the race with GORM's own defaulting. The hook sets a non-zero value first, so the column default never applies.

Verify the hook actually fires on the edit path as written — if the edit path uses Updates with a map or a selective column set rather than a struct Save, confirm the hook still runs and the rewritten value is what gets persisted. Say in the PR body which save call you verified and how.

3. The reaper

retention.go currently treats <= 0 as retain-forever. With the sentinel stored, that branch is no longer reachable from normal data: 365000 days is a positive number, so the reaper would compute a cutoff a thousand years in the past and issue a pointless DELETE that matches nothing on every sweep for every retain-forever webhook.

Make the reaper recognise RetentionDays >= RetentionForeverDays and skip the webhook entirely, before building any query. Keep the existing <= 0 guard as defense-in-depth for any row written before this change.

4. Form handling

parseRetention currently does if err == nil && v > 0, which silently drops both unparseable input and zero. Both behaviours are now wrong:

0 must be honoured and become retain-forever.
An unparseable or negative value must not be silently ignored. This is user-facing form input, so the fail-loud response is a 400 with a clear message re-rendering the form, not a startup abort — do not copy #80's treatment literally here.
An empty field keeps the current "leave unchanged" behaviour on edit, and the 30-day default on create.

Apply the same rules to the create path so create and edit cannot drift.

5. UI — keep it minimal, #57 owns the real cleanup

Three concrete things, no redesign:

min="1" must become min="0" in templates/sources_new.html and templates/source_edit.html, with a hint that 0 means retain forever. Otherwise the value is unreachable from the UI, which is the entire bug.
max="365" must go (or rise to the sentinel). This is a trap: the edit form pre-fills value="{{.Webhook.RetentionDays}}", so a retain-forever webhook renders 365000 into an input capped at 365, and browser validation blocks the user from saving any edit to that webhook until they change retention. Verify a retain-forever webhook can round-trip through the edit form untouched.
Display "forever", not "365000d". Add a method on the model (e.g. RetainsForever() bool, or a label helper) and use it in templates/sources_list.html and templates/source_detail.html. Do not put the magic number in a template.

6. Consolidate the duplicated default

defaultRetentionDays = 30 in internal/handlers/handlers.go and gorm:"default:30" on the model are two sources of truth for one policy. Fold them into one exported constant in internal/database alongside the sentinel, and reference it from both. If the GORM struct tag cannot take a constant, leave the tag but add a comment tying it to the constant and a test asserting the two agree.

7. Tests

Creating a webhook with retention_days=0 through the handler persists RetentionForeverDays, not 30. This is the core regression test — it must fail without the hook.
Editing an existing webhook to 0 persists the sentinel.
The reaper skips a webhook at the sentinel and issues no delete, while still reaping a normal 30-day webhook in the same sweep.
A retain-forever webhook round-trips through the edit form without its retention changing.
Unparseable and negative form values produce a 400, not a silent default.
Creating without the field still yields 30.

8. Docs

README: document 0 meaning retain forever, and that it is stored as 365 * 1000 days.
TODO.md updated in the same commit as the code.

Definition of done

Everything in the issue's Definition of done, plus items 1-8 above, make check green via the repo's own entrypoints only, .golangci.yml untouched, single commit whose title ends with (closes #79), no attribution trailers.

## Implementation requirements Baseline: `main` @ `4f5ecb1`. @sneak's decision in the comments above is the design and is not up for reinterpretation: **`365 * 1000` days is the retain-forever value, and an attempt to set `retention_days` to 0 is rewritten to that value on insert and update.** ### Current state, confirmed by reading - `internal/database/model_webhook.go:12` — `RetentionDays int \`gorm:"default:30" json:"retentionDays"\``. The GORM column default is why a zero value becomes 30 on `Create`. - `internal/handlers/handlers.go:30` — `defaultRetentionDays = 30`, a second source of truth for the same 30. - `internal/handlers/source_management.go:145-185` — the create path; `internal/handlers/source_management.go:445-461` — `parseRetention`, the edit path. - `internal/database/retention.go:143` — `if wh.RetentionDays <= 0 { continue }`, the current retain-forever branch. - Templates: `templates/sources_new.html:31` and `templates/source_edit.html:31` both use `min="1" max="365"`; `templates/sources_list.html:28` renders `{{.RetentionDays}}d retention`; `templates/source_detail.html:184` renders `Retention: {{.Webhook.RetentionDays}} days`. ### 1. The sentinel Add one exported constant in `internal/database` — e.g. `RetentionForeverDays = 365 * 1000` — written as that expression, not as a bare `365000`, so its meaning is self-evident. It is the single source of truth; nothing else may hardcode the number. ### 2. Rewrite on insert and update Implement the rewrite as a GORM `BeforeSave` hook on `Webhook` so it fires on create **and** update and cannot be bypassed by a future call site (a REST API is on the roadmap). Any `RetentionDays <= 0` becomes `RetentionForeverDays`. Note the interaction that makes the hook necessary rather than optional: the `gorm:"default:30"` tag substitutes 30 for a zero value at insert time, so a rewrite that happens anywhere later than the hook loses the race with GORM's own defaulting. The hook sets a non-zero value first, so the column default never applies. Verify the hook actually fires on the edit path as written — if the edit path uses `Updates` with a map or a selective column set rather than a struct `Save`, confirm the hook still runs and the rewritten value is what gets persisted. Say in the PR body which save call you verified and how. ### 3. The reaper `retention.go` currently treats `<= 0` as retain-forever. With the sentinel stored, that branch is no longer reachable from normal data: 365000 days is a positive number, so the reaper would compute a cutoff a thousand years in the past and issue a pointless `DELETE` that matches nothing on every sweep for every retain-forever webhook. Make the reaper recognise `RetentionDays >= RetentionForeverDays` and skip the webhook entirely, before building any query. **Keep the existing `<= 0` guard** as defense-in-depth for any row written before this change. ### 4. Form handling `parseRetention` currently does `if err == nil && v > 0`, which silently drops both unparseable input and zero. Both behaviours are now wrong: - `0` must be honoured and become retain-forever. - An unparseable or negative value must not be silently ignored. This is user-facing form input, so the fail-loud response is a 400 with a clear message re-rendering the form, **not** a startup abort — do not copy #80's treatment literally here. - An empty field keeps the current "leave unchanged" behaviour on edit, and the 30-day default on create. Apply the same rules to the create path so create and edit cannot drift. ### 5. UI — keep it minimal, #57 owns the real cleanup Three concrete things, no redesign: - **`min="1"` must become `min="0"`** in `templates/sources_new.html` and `templates/source_edit.html`, with a hint that 0 means retain forever. Otherwise the value is unreachable from the UI, which is the entire bug. - **`max="365"` must go** (or rise to the sentinel). This is a trap: the edit form pre-fills `value="{{.Webhook.RetentionDays}}"`, so a retain-forever webhook renders `365000` into an input capped at 365, and browser validation blocks the user from saving **any** edit to that webhook until they change retention. Verify a retain-forever webhook can round-trip through the edit form untouched. - **Display "forever", not "365000d".** Add a method on the model (e.g. `RetainsForever() bool`, or a label helper) and use it in `templates/sources_list.html` and `templates/source_detail.html`. Do not put the magic number in a template. ### 6. Consolidate the duplicated default `defaultRetentionDays = 30` in `internal/handlers/handlers.go` and `gorm:"default:30"` on the model are two sources of truth for one policy. Fold them into one exported constant in `internal/database` alongside the sentinel, and reference it from both. If the GORM struct tag cannot take a constant, leave the tag but add a comment tying it to the constant and a test asserting the two agree. ### 7. Tests - Creating a webhook with `retention_days=0` through the **handler** persists `RetentionForeverDays`, not 30. This is the core regression test — it must fail without the hook. - Editing an existing webhook to `0` persists the sentinel. - The reaper skips a webhook at the sentinel and issues no delete, while still reaping a normal 30-day webhook in the same sweep. - A retain-forever webhook round-trips through the edit form without its retention changing. - Unparseable and negative form values produce a 400, not a silent default. - Creating without the field still yields 30. ### 8. Docs - README: document `0` meaning retain forever, and that it is stored as `365 * 1000` days. - `TODO.md` updated in the **same commit** as the code. ### Definition of done Everything in the issue's Definition of done, plus items 1-8 above, `make check` green via the repo's own entrypoints only, `.golangci.yml` untouched, single commit whose title ends with ` (closes #79)`, no attribution trailers.

clawbot commented

2026-08-09 04:12:27 +02:00

Implementation plan

Branch issue-79-retention-forever off main @ 4f5ecb1, single commit ending (closes #79).

1. `internal/database/model_webhook.go` — sentinel, default, hook, display helpers

const RetentionForeverDays = 365 * 1000 (written as the expression), the single source of truth for the retain-forever value.
const DefaultRetentionDays = 30, the single source of truth for the 30-day default. The gorm:"default:30" struct tag cannot take a constant, so it stays, with a comment tying it to DefaultRetentionDays and a reflect-based test asserting the tag literally reads default:30.
func (w *Webhook) BeforeSave(*gorm.DB) error rewriting any RetentionDays <= 0 to RetentionForeverDays. It is a hook rather than call-site logic precisely because the default:30 tag substitutes 30 for a zero value during GORM's own create callback, which runs after BeforeSave — the hook writes a non-zero value first, so the column default never fires.
func (w Webhook) RetainsForever() bool — true for <= 0 (legacy rows) or >= RetentionForeverDays. Value receiver so templates can call it on the non-pointer database.Webhook embedded in WebhookListItem.
func (w Webhook) RetentionLabel() string — "forever" or "30 days", so no template holds the magic number.

2. `internal/database/retention.go` — reaper

sweep skips on wh.RetainsForever() before touching the DB manager or building any query. That single call keeps the pre-existing <= 0 guard as defense-in-depth for rows written before this change, and adds sentinel recognition, so a retain-forever webhook issues no DELETE at all rather than a no-op delete against a cutoff a thousand years in the past.

3. `internal/handlers/` — form handling

Delete defaultRetentionDays from handlers.go; reference database.DefaultRetentionDays.
One shared parseRetentionDays(raw string, fallback int) (int, error), used by both create and edit so they cannot drift:
- empty -> fallback (create passes database.DefaultRetentionDays, edit passes the webhook's current value, preserving "leave unchanged")
- 0 -> returned as 0 and rewritten by BeforeSave; the handler does not know the sentinel
- unparseable or negative -> error; the handler responds 400 and re-renders the form with the message, not a silent default and not a startup abort
parseRetention (the current silent-drop helper) is replaced.

4. Templates

templates/sources_new.html and templates/source_edit.html: min="1" -> min="0", and max="365" removed. The max removal is not cosmetic: the edit form pre-fills value="{{.Webhook.RetentionDays}}", so a retain-forever webhook renders 365000 into an input capped at 365 and browser validation would block saving any edit to that webhook. A hint line states that 0 means retain forever.
templates/sources_list.html and templates/source_detail.html render RetentionLabel instead of the raw number.
Only existing utility classes are reused (input, label, badge-info, text-xs text-gray-500 mt-1), so the make css Tailwind build is not required.

5. Tests

In internal/handlers (handler level, so they fail without the hook) and internal/database:

create with retention_days=0 persists RetentionForeverDays, not 30 — the core regression test
create with the field absent still persists 30
create and edit with unparseable and with negative values -> 400, stored value unchanged
edit to 0 persists the sentinel
a retain-forever webhook round-trips through the edit form (submit the pre-filled 365000 back) with its retention unchanged
reaper: sentinel webhook untouched while a 30-day webhook is reaped in the same sweep
the gorm tag default agrees with DefaultRetentionDays

6. Docs

README retention_days row and prose document 0 meaning retain forever and that it is stored as 365 * 1000 days. TODO.md updated in the same commit.

Verification with make fmt, make check, and script/cibuild only.

## Implementation plan Branch `issue-79-retention-forever` off `main` @ `4f5ecb1`, single commit ending ` (closes #79)`. ### 1. `internal/database/model_webhook.go` — sentinel, default, hook, display helpers - `const RetentionForeverDays = 365 * 1000` (written as the expression), the single source of truth for the retain-forever value. - `const DefaultRetentionDays = 30`, the single source of truth for the 30-day default. The `gorm:"default:30"` struct tag cannot take a constant, so it stays, with a comment tying it to `DefaultRetentionDays` and a reflect-based test asserting the tag literally reads `default:30`. - `func (w *Webhook) BeforeSave(*gorm.DB) error` rewriting any `RetentionDays <= 0` to `RetentionForeverDays`. It is a hook rather than call-site logic precisely because the `default:30` tag substitutes 30 for a zero value during GORM's own create callback, which runs after `BeforeSave` — the hook writes a non-zero value first, so the column default never fires. - `func (w Webhook) RetainsForever() bool` — true for `<= 0` (legacy rows) or `>= RetentionForeverDays`. Value receiver so templates can call it on the non-pointer `database.Webhook` embedded in `WebhookListItem`. - `func (w Webhook) RetentionLabel() string` — `"forever"` or `"30 days"`, so no template holds the magic number. ### 2. `internal/database/retention.go` — reaper `sweep` skips on `wh.RetainsForever()` before touching the DB manager or building any query. That single call keeps the pre-existing `<= 0` guard as defense-in-depth for rows written before this change, and adds sentinel recognition, so a retain-forever webhook issues no `DELETE` at all rather than a no-op delete against a cutoff a thousand years in the past. ### 3. `internal/handlers/` — form handling - Delete `defaultRetentionDays` from `handlers.go`; reference `database.DefaultRetentionDays`. - One shared `parseRetentionDays(raw string, fallback int) (int, error)`, used by both create and edit so they cannot drift: - empty -> `fallback` (create passes `database.DefaultRetentionDays`, edit passes the webhook's current value, preserving "leave unchanged") - `0` -> returned as `0` and rewritten by `BeforeSave`; the handler does not know the sentinel - unparseable or negative -> error; the handler responds 400 and re-renders the form with the message, not a silent default and not a startup abort - `parseRetention` (the current silent-drop helper) is replaced. ### 4. Templates - `templates/sources_new.html` and `templates/source_edit.html`: `min="1"` -> `min="0"`, and **`max="365"` removed**. The `max` removal is not cosmetic: the edit form pre-fills `value="{{.Webhook.RetentionDays}}"`, so a retain-forever webhook renders `365000` into an input capped at 365 and browser validation would block saving any edit to that webhook. A hint line states that 0 means retain forever. - `templates/sources_list.html` and `templates/source_detail.html` render `RetentionLabel` instead of the raw number. - Only existing utility classes are reused (`input`, `label`, `badge-info`, `text-xs text-gray-500 mt-1`), so the `make css` Tailwind build is not required. ### 5. Tests In `internal/handlers` (handler level, so they fail without the hook) and `internal/database`: - create with `retention_days=0` persists `RetentionForeverDays`, not 30 — the core regression test - create with the field absent still persists 30 - create and edit with unparseable and with negative values -> 400, stored value unchanged - edit to `0` persists the sentinel - a retain-forever webhook round-trips through the edit form (submit the pre-filled `365000` back) with its retention unchanged - reaper: sentinel webhook untouched while a 30-day webhook is reaped in the same sweep - the `gorm` tag default agrees with `DefaultRetentionDays` ### 6. Docs README `retention_days` row and prose document `0` meaning retain forever and that it is stored as `365 * 1000` days. `TODO.md` updated in the same commit. Verification with `make fmt`, `make check`, and `script/cibuild` only.

clawbot referenced this issue from a commit

2026-08-09 04:32:39 +02:00

Allow retention_days of 0 to mean retain forever (closes #79)

clawbot referenced a pull request that will close this issue

2026-08-09 04:33:24 +02:00

Allow retention_days of 0 to mean retain forever (closes #79) #96

clawbot referenced this issue from a commit

2026-08-09 04:57:39 +02:00

Allow retention_days of 0 to mean retain forever (closes #79)

clawbot referenced this issue

2026-08-09 05:11:44 +02:00

Follow-ups from the PR #96 re-review: retention bound docs and untested normalisation #99