sneak/webhooker
1
0
Fork 0
Code Issues 11 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

The withRetry bookkeeping-failure branch is unpinned: removing its error check leaves the suite green #285

New Issue
Closed
opened 2026-08-24 03:49:30 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-24 03:49:30 +02:00
Collaborator
Copy Link
Copy Source

Found by mutation testing during the second review of #263.

internal/delivery/target_http.go:130-141 — the withRetry bookkeeping-failure branch. Replacing its recordResult error check with _ = leaves the ENTIRE internal/delivery suite green. Its fire-and-forget twin at :80-86 IS pinned by a test.

Why this particular arm matters: it is the one that creates the retrying-with-a-successful-result state, which is exactly what findings 1 and 2 of that PR's first review were about — a delivery whose send succeeded but whose bookkeeping write failed. #263 fixed the handling of that state and added reconciliation for it. The handling is now correct and was verified live; what is missing is a test that would notice if it regressed.

Four other guards in that PR were mutation-checked and all went red when removed. This is the one that did not.

Definition of done

  • A test that fails when withRetry's bookkeeping-failure branch stops checking its recordResult error. Mutation-verify it: make the mutation, confirm red; restore, confirm green. State that you did so.
  • While there, check the sibling paths in the same file for the same gap — a guard that no test pins is easy to have more than one of.

Not milestoned: the behaviour is correct as it stands and was verified by execution across both reader and no-reader arms. This is regression insurance for a mechanism that took two review rounds to get right, which is precisely the kind of code that should not be left unpinned.

Found by mutation testing during the second review of https://git.eeqj.de/sneak/webhooker/pulls/263. `internal/delivery/target_http.go:130-141` — the `withRetry` bookkeeping-failure branch. Replacing its `recordResult` error check with `_ =` leaves the ENTIRE `internal/delivery` suite green. Its fire-and-forget twin at `:80-86` IS pinned by a test. Why this particular arm matters: it is the one that creates the retrying-with-a-successful-result state, which is exactly what findings 1 and 2 of that PR's first review were about — a delivery whose send succeeded but whose bookkeeping write failed. https://git.eeqj.de/sneak/webhooker/pulls/263 fixed the handling of that state and added reconciliation for it. The handling is now correct and was verified live; what is missing is a test that would notice if it regressed. Four other guards in that PR were mutation-checked and all went red when removed. This is the one that did not. ## Definition of done - A test that fails when `withRetry`'s bookkeeping-failure branch stops checking its `recordResult` error. Mutation-verify it: make the mutation, confirm red; restore, confirm green. State that you did so. - While there, check the sibling paths in the same file for the same gap — a guard that no test pins is easy to have more than one of. Not milestoned: the behaviour is correct as it stands and was verified by execution across both reader and no-reader arms. This is regression insurance for a mechanism that took two review rounds to get right, which is precisely the kind of code that should not be left unpinned.
clawbot commented 2026-10-02 17:53:48 +02:00
Author
Collaborator
Copy Link
Copy Source

Plan. The line numbers are from September; check against next first. Add a test that fails when withRetry's bookkeeping-failure branch in internal/delivery/target_http.go stops checking its recordResult error, driving a send that succeeds while recording the result fails. Then check every other error check in that file the same way (remove it, run the package tests) and pin any that no test catches. The PR lists each guard checked and how it is pinned. Test change only, unless a guard turns out to be wrong.

Model: opus-5-5

Plan. The line numbers are from September; check against `next` first. Add a test that fails when `withRetry`'s bookkeeping-failure branch in `internal/delivery/target_http.go` stops checking its `recordResult` error, driving a send that succeeds while recording the result fails. Then check every other error check in that file the same way (remove it, run the package tests) and pin any that no test catches. The PR lists each guard checked and how it is pinned. Test change only, unless a guard turns out to be wrong. Model: opus-5-5
clawbot commented 2026-10-02 18:28:16 +02:00
Author
Collaborator
Copy Link
Copy Source

Opened #458. Tests only: withRetry's check on writing the delivery result, and six other error checks in internal/delivery/target_http.go that could be removed without any test noticing, now each fail a test when removed. The PR lists each one.

Judgement call: the checks in remainingBackoff and backoffElapsed stay unpinned; removing them gives the same answer, so no test can tell.

Model: opus-5-5

Opened https://git.eeqj.de/sneak/webhooker/pulls/458. Tests only: `withRetry`'s check on writing the delivery result, and six other error checks in `internal/delivery/target_http.go` that could be removed without any test noticing, now each fail a test when removed. The PR lists each one. Judgement call: the checks in `remainingBackoff` and `backoffElapsed` stay unpinned; removing them gives the same answer, so no test can tell. Model: opus-5-5
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
critical
Extremely important: data loss, a security hole, production down, or basic use impossible
 merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
clawbot sneak (Jeffrey Paul)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#285