From 373a2f828daaa68748af44d2cc7495dfcff97766 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 13 Oct 2026 09:57:24 +0200 Subject: [PATCH] Describe code in its own terms, not by an earlier build's data (closes #522) Several comments justified code by what an earlier build would have left in a database or on disk. Each now says what the code does: reserveSQLiteFile sets the mode on every open whoever created the file, and expired treats a session lacking a deadline's timestamp as expired. The comments on retention, resubmitted events, archive pruning, target headers, the redactor and the delivery result view drop their mentions of older rows. parseMaxRetries loses its paragraph on targets saved before the ceiling existed, and neither BeforeSave nor retentionCutoff cites a migration. Comments only. Model: opus-5-5 --- internal/database/model_event.go | 3 +-- internal/database/model_webhook.go | 6 +++--- internal/database/retention.go | 2 +- internal/database/sqlite_open.go | 5 ++--- internal/delivery/archive_sweeper.go | 3 +-- internal/delivery/target_headers.go | 4 ++-- internal/delivery/target_redact.go | 6 +++--- internal/handlers/delivery_result_view.go | 14 ++++++-------- internal/handlers/target_retries.go | 7 ------- internal/session/session.go | 7 +++---- 10 files changed, 22 insertions(+), 35 deletions(-) diff --git a/internal/database/model_event.go b/internal/database/model_event.go index b50fe32..0279230 100644 --- a/internal/database/model_event.go +++ b/internal/database/model_event.go @@ -45,8 +45,7 @@ type Event struct { // ResubmittedFromID names the event this one was copied from by // an operator resubmit. It is nil for an event that arrived on - // the receiver, which is every event created before the column - // existed. It is not a foreign key: the source event can be + // the receiver. It is not a foreign key: the source event can be // reaped by retention while its copies remain, and the id is // kept as the record of where the copy came from either way. ResubmittedFromID *string `gorm:"type:uuid;index:idx_events_resubmitted_from_id,priority:1;index:idx_events_entrypoint_id,priority:3" json:"resubmittedFromId,omitempty"` diff --git a/internal/database/model_webhook.go b/internal/database/model_webhook.go index c6835e1..bc0de7f 100644 --- a/internal/database/model_webhook.go +++ b/internal/database/model_webhook.go @@ -82,7 +82,7 @@ type Webhook struct { // value while building the insert statement, which runs after // BeforeSave; rewriting any later than this loses that race and the // row lands at 30 days. Living on the model also means a future call -// site — a REST API, a fixture, a migration — cannot bypass it. +// site, such as a REST API or a fixture, cannot bypass it. func (w *Webhook) BeforeSave(_ *gorm.DB) error { if w.RetentionDays <= 0 { w.RetentionDays = RetentionForeverDays @@ -98,8 +98,8 @@ func (w *Webhook) BeforeSave(_ *gorm.DB) error { // are exempt from reaping. // // It accepts the RetentionForeverDays sentinel written by BeforeSave -// and, defensively, the non-positive values that rows written before -// the sentinel existed may still carry. +// and, defensively, any non-positive value, which BeforeSave treats +// as a request to retain forever. func retainsForever(retentionDays int) bool { return retentionDays <= 0 || retentionDays >= RetentionForeverDays diff --git a/internal/database/retention.go b/internal/database/retention.go index 7ee7d75..5265ea9 100644 --- a/internal/database/retention.go +++ b/internal/database/retention.go @@ -253,7 +253,7 @@ func (r *RetentionReaper) reapWebhook( // row in the database: the sweep then deletes every event, delivery, // and delivery result, including ones created seconds ago. Rejecting // out-of-range input at the form is the primary guard; saturating here -// means an old row, a migration, or a future call site cannot turn a +// means a value that reaches the row by any other path cannot turn a // too-large retention into total data loss. func retentionCutoff( now time.Time, diff --git a/internal/database/sqlite_open.go b/internal/database/sqlite_open.go index e73ce57..ab6d368 100644 --- a/internal/database/sqlite_open.go +++ b/internal/database/sqlite_open.go @@ -118,9 +118,8 @@ const SQLiteFilePerm fs.FileMode = 0o600 // SQLite still reports the absence rather than this function // materializing an empty database the caller asked not to create. // -// Chmod of a file that already exists is what tightens a data -// directory an earlier build left at 0644 — including a developer's -// own scratch directory — without any migration machinery. +// The mode is set on every open, whoever created the file, so a +// database or sidecar found at a wider mode is tightened too. func reserveSQLiteFile(path string, create bool) error { if create { // gosec G304: the path is the database file the caller asked diff --git a/internal/delivery/archive_sweeper.go b/internal/delivery/archive_sweeper.go index 838dddf..1a6b87a 100644 --- a/internal/delivery/archive_sweeper.go +++ b/internal/delivery/archive_sweeper.go @@ -201,8 +201,7 @@ func (s *ArchiveSweeper) sweep(ctx context.Context) { // sweepTarget prunes the archive of a single database target. // A missing, empty, or "never" expiry parses as a zero duration -// and is skipped entirely, so those archives keep exactly the -// behaviour they had before the sweep existed. +// and is skipped entirely, so those archives are never pruned. func (s *ArchiveSweeper) sweepTarget(target *database.Target) { expiry, err := parseArchiveExpiry(target.Config) if err != nil { diff --git a/internal/delivery/target_headers.go b/internal/delivery/target_headers.go index 20e3c9b..ca2b069 100644 --- a/internal/delivery/target_headers.go +++ b/internal/delivery/target_headers.go @@ -82,8 +82,8 @@ func isReservedTargetHeader(name string) bool { // one pair silently overwriting the other. // // An input with no pairs yields an empty map, which omitempty drops -// from the stored config: a target configured with no headers keeps -// the same config JSON it had before this field existed. +// from the stored config, so a target configured with no headers +// stores no headers key at all. func ParseTargetHeaders(raw string) (map[string]string, error) { headers := make(map[string]string) diff --git a/internal/delivery/target_redact.go b/internal/delivery/target_redact.go index 267e584..31d4867 100644 --- a/internal/delivery/target_redact.go +++ b/internal/delivery/target_redact.go @@ -13,9 +13,9 @@ import ( const RedactionMarker = "(redacted)" // Redactor removes one target's own credential material from -// text that target's remote peer chose: a delivery response -// body, or a delivery error stored before the delivery path -// learned to mask the URLs it embeds. +// a delivery response body, whose text the target's remote +// peer chose, and from a stored delivery error as a second +// line behind the delivery path's own URL masking. // // It removes byte-identical echoes of strings taken from the // target's stored configuration, and nothing else. Anything diff --git a/internal/handlers/delivery_result_view.go b/internal/handlers/delivery_result_view.go index f635b92..76140c5 100644 --- a/internal/handlers/delivery_result_view.go +++ b/internal/handlers/delivery_result_view.go @@ -12,13 +12,12 @@ import ( // // The bound is enforced here and in SQL, because this page's // memory profile must not depend on a constant in another -// package staying where it is, and because rows predating the -// delivery engine's own cap or restored from an archive are -// not covered by it at all. +// package staying where it is, or on every stored row having +// been written by the delivery engine under its own cap. // // It happens to equal that engine cap, so a row written by the -// current engine reaches this one exactly and is never cut -// twice. Nothing here may assume the two differ: see view. +// engine reaches this one exactly and is never cut twice. +// Nothing here may assume the two differ: see view. const maxRenderedResponseBytes = 4096 // deliveryResultColumns is the delivery attempt projection. @@ -40,9 +39,8 @@ const deliveryResultColumns = "delivery_id, attempt_num, created_at, " + // // Both Error and ResponseBody have been through the target's // Redactor. The engine already masks the URL out of the -// errors it stores, so for errors this is a second line -// covering rows written before it did; for response bodies it -// is the only line, and its reach is what +// errors it stores, so for errors this is a second line; for +// response bodies it is the only line, and its reach is what // delivery.Redactor documents. type DeliveryResultView struct { AttemptNum int diff --git a/internal/handlers/target_retries.go b/internal/handlers/target_retries.go index 15979c3..6e68916 100644 --- a/internal/handlers/target_retries.go +++ b/internal/handlers/target_retries.go @@ -49,13 +49,6 @@ var ( // working retry configuration over a typo. A default answers a // question that was not asked; it never answers one that was asked // badly. -// -// A target stored with a count above the ceiling before this -// validation existed keeps rendering and keeps delivering — nothing -// clamps the row. Re-saving it from the edit form does have to bring -// it into range, because the form submits the pre-filled value back -// and accepting it would be the ceiling not applying to the edit -// path. The 400 names the ceiling, so the fix is one field. func parseMaxRetries(raw string, fallback int) (int, error) { raw = strings.TrimSpace(raw) if raw == "" { diff --git a/internal/session/session.go b/internal/session/session.go index 5335000..1422bf2 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -441,10 +441,9 @@ func (s *Session) Regenerate( // // Whichever comes first ends the session. // -// A session that claims to be authenticated but carries no -// timestamps predates this check; it is treated as expired so the -// user re-authenticates rather than being granted an unbounded -// session. +// A session that claims to be authenticated but lacks the timestamp +// a deadline is measured from is treated as expired, so the user +// re-authenticates rather than being granted an unbounded session. func (s *Session) expired(sess *sessions.Session) bool { now := s.now()