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

New Issue

clawbot · 2026-08-09T05:11:44+02:00

clawbot commented

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

Tracking issue for the non-blocking items from the independent re-review of PR #96 (#79), so they do not become untracked. None blocked that merge. They are small enough to land as one tidy commit.

Items

README.md is inaccurate about the upper bound. It says a retention value above the ceiling "is rejected with a 400". That is only true for the finite unsafe band (106751, 365000). A value of 365000 or above is accepted and means retain forever. One clarifying sentence: values above MaxFiniteRetentionDays but below the retain-forever sentinel are rejected; the sentinel and anything above it mean forever.
The normalisation arm has no test behind it. Mutating return database.RetentionForeverDays, nil to return v, nil in parseRetentionDays leaves the entire suite green. This is the one piece of the new boundary logic with no coverage, and it is precisely the arm the author deliberately deviated on (folding >= RetentionForeverDays to the sentinel rather than 400). Add a test pinning 365001 → stored 365000.
errInvalidRetention is still never an errors.Is operand. Only errRetentionTooLarge is matched. The rework comment's claim that it is "now used that way" overstates it. Either match on it or correct the comment — an error value that exists only to be returned and never compared is a trap for the next reader.
The sweep skip is fully redundant with retentionCutoff. Replacing the skip with if false leaves the suite green, because retentionCutoff already returns false for retain-forever. Harmless defense-in-depth, but either give it a test that distinguishes it from the inner guard or drop it and rely on the one that is actually load-bearing.
The HTML-escaping test uses plain ASCII, so it would not catch a future raw-HTML regression in the create-form refill. The escaping itself was verified correct by execution during review (a x"><script> payload renders fully entity-escaped in both the value=" attribute and the textarea) — this is only about the test being weaker than the check it stands for.
A hypothetical legacy negative retention_days row 400s on an unchanged edit-form submit. No such row can be created now that the BeforeSave hook normalises them, so this is only reachable via direct DB manipulation. Worth a line of thought about whether the edit path should normalise rather than reject.

Definition of done

Items 1-3 fixed; item 4 either tested or removed; items 5-6 addressed or explicitly declined with reasoning.
make check green via the repo's own entrypoints; .golangci.yml untouched.

Tracking issue for the non-blocking items from the independent re-review of PR #96 (#79), so they do not become untracked. None blocked that merge. They are small enough to land as one tidy commit. ## Items 1. **`README.md` is inaccurate about the upper bound.** It says a retention value above the ceiling "is rejected with a 400". That is only true for the finite unsafe band `(106751, 365000)`. A value of `365000` or above is *accepted* and means retain forever. One clarifying sentence: values above `MaxFiniteRetentionDays` but below the retain-forever sentinel are rejected; the sentinel and anything above it mean forever. 2. **The normalisation arm has no test behind it.** Mutating `return database.RetentionForeverDays, nil` to `return v, nil` in `parseRetentionDays` leaves the **entire suite green**. This is the one piece of the new boundary logic with no coverage, and it is precisely the arm the author deliberately deviated on (folding `>= RetentionForeverDays` to the sentinel rather than 400). Add a test pinning `365001` → stored `365000`. 3. **`errInvalidRetention` is still never an `errors.Is` operand.** Only `errRetentionTooLarge` is matched. The rework comment's claim that it is "now used that way" overstates it. Either match on it or correct the comment — an error value that exists only to be returned and never compared is a trap for the next reader. 4. **The `sweep` skip is fully redundant with `retentionCutoff`.** Replacing the skip with `if false` leaves the suite green, because `retentionCutoff` already returns `false` for retain-forever. Harmless defense-in-depth, but either give it a test that distinguishes it from the inner guard or drop it and rely on the one that is actually load-bearing. 5. **The HTML-escaping test uses plain ASCII**, so it would not catch a future raw-HTML regression in the create-form refill. The escaping itself was verified correct by execution during review (a `x"><script>` payload renders fully entity-escaped in both the `value="` attribute and the textarea) — this is only about the test being weaker than the check it stands for. 6. **A hypothetical legacy negative `retention_days` row 400s on an unchanged edit-form submit.** No such row can be created now that the `BeforeSave` hook normalises them, so this is only reachable via direct DB manipulation. Worth a line of thought about whether the edit path should normalise rather than reject. ## Definition of done - Items 1-3 fixed; item 4 either tested or removed; items 5-6 addressed or explicitly declined with reasoning. - `make check` green via the repo's own entrypoints; `.golangci.yml` untouched.

clawbot referenced this issue

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

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

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: sneak/webhooker#99