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.
Plan. The items are from August; check each against next first, fix only what is still true, and say in the PR which were already settled.
The README says exactly which retention values are refused and which mean keep forever.
A test pins that a value at or above the keep-forever sentinel is stored as the sentinel; changing that branch to return the value as given must fail it.
Each retention error value the code defines is either compared somewhere or removed, and no comment claims otherwise.
The redundant keep-forever skip in the sweep is removed, leaving the check in retentionCutoff, or given a test that tells it apart.
The create-form refill test uses a value with HTML-special characters and checks it is escaped.
Declined: a negative retention row cannot be created, and this pre-1.0 service carries no handling for rows made by hand.
Model: opus-5-5
Plan. The items are from August; check each against `next` first, fix only what is still true, and say in the PR which were already settled.
1. The README says exactly which retention values are refused and which mean keep forever.
2. A test pins that a value at or above the keep-forever sentinel is stored as the sentinel; changing that branch to return the value as given must fail it.
3. Each retention error value the code defines is either compared somewhere or removed, and no comment claims otherwise.
4. The redundant keep-forever skip in the sweep is removed, leaving the check in `retentionCutoff`, or given a test that tells it apart.
5. The create-form refill test uses a value with HTML-special characters and checks it is escaped.
6. Declined: a negative retention row cannot be created, and this pre-1.0 service carries no handling for rows made by hand.
Model: opus-5-5
Item 1 was already settled on next: the README states the refused band and that the sentinel and above mean retain forever.
Item 2: a test now pins that a value above the retain-forever sentinel is stored as the sentinel.
Item 3: parseRetentionDays returns the form's message directly, so both retention error values and the function that mapped them to messages are removed.
Item 4: the sweep's own retain-forever skip is removed; retentionCutoff is now asked before the webhook's database is opened.
Item 5: the create-form refill test uses HTML-special characters and checks they come back escaped.
Item 6: declined, as planned.
Judgement call: item 3 removes the error values rather than comparing them, so the retention and max-retries parsers now differ in shape.
Model: opus-5-5
Opened https://git.eeqj.de/sneak/webhooker/pulls/446.
- Item 1 was already settled on `next`: the README states the refused band and that the sentinel and above mean retain forever.
- Item 2: a test now pins that a value above the retain-forever sentinel is stored as the sentinel.
- Item 3: `parseRetentionDays` returns the form's message directly, so both retention error values and the function that mapped them to messages are removed.
- Item 4: the sweep's own retain-forever skip is removed; `retentionCutoff` is now asked before the webhook's database is opened.
- Item 5: the create-form refill test uses HTML-special characters and checks they come back escaped.
- Item 6: declined, as planned.
Judgement call: item 3 removes the error values rather than comparing them, so the retention and max-retries parsers now differ in shape.
Model: opus-5-5
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
Plan. The items are from August; check each against
nextfirst, fix only what is still true, and say in the PR which were already settled.retentionCutoff, or given a test that tells it apart.Model: opus-5-5
Opened #446.
next: the README states the refused band and that the sentinel and above mean retain forever.parseRetentionDaysreturns the form's message directly, so both retention error values and the function that mapped them to messages are removed.retentionCutoffis now asked before the webhook's database is opened.Judgement call: item 3 removes the error values rather than comparing them, so the retention and max-retries parsers now differ in shape.
Model: opus-5-5