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.
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.
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.mdis 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 of365000or above is accepted and means retain forever. One clarifying sentence: values aboveMaxFiniteRetentionDaysbut 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, niltoreturn v, nilinparseRetentionDaysleaves 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>= RetentionForeverDaysto the sentinel rather than 400). Add a test pinning365001→ stored365000.errInvalidRetentionis still never anerrors.Isoperand. OnlyerrRetentionTooLargeis 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
sweepskip is fully redundant withretentionCutoff. Replacing the skip withif falseleaves the suite green, becauseretentionCutoffalready returnsfalsefor 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 thevalue="attribute and the textarea) — this is only about the test being weaker than the check it stands for.A hypothetical legacy negative
retention_daysrow 400s on an unchanged edit-form submit. No such row can be created now that theBeforeSavehook 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
make checkgreen via the repo's own entrypoints;.golangci.ymluntouched.clawbot referenced this issue2026-08-17 22:44:00 +02:00