Problem
retention_prune_sql_deletes_offset_tail (line 374) asserts internal SQL fragments emitted by retention_prune_sql() (line 209) rather than any observable runtime outcome:
// crates/core/src/storage/history.rs lines 374–396
#[test]
fn retention_prune_sql_deletes_offset_tail() {
let sql = HistoryDb::retention_prune_sql();
assert!(sql.contains("LIMIT -1 OFFSET ?1"), …);
assert!(sql.contains("WHERE id IN ("), …);
assert!(!sql.to_ascii_lowercase().contains("not in"), …);
assert!(sql.contains("ORDER BY created_at DESC, id DESC"), …);
}The test calls retention_prune_sql() directly and string-matches on "LIMIT -1 OFFSET ?1", "WHERE id IN (", "NOT IN", and "ORDER BY created_at DESC, id DESC".
Any semantics-preserving rewrite of the helper (e.g. switching to a NOT IN anti-join, a CTE, or a DELETE … RETURNING-based approach) would break this test even if the three behavior tests below continue to pass.
Behavior already covered by adjacent tests:
| Test |
Lines |
What it verifies |
retention_prunes_to_max_rows_and_keeps_newest |
322–371 |
Row count == cap; oldest survivor is correct |
retention_prune_helper_deletes_zero_when_under_cap |
399–437 |
No delete when at- or under-cap |
retention_prune_helper_deletes_oldest_tail_over_cap |
440–481 |
Exactly overflow rows deleted; oldest survivor correct |
Why it matters
- Refactor friction. Any SQL simplification (relevant if the
COUNT(*) guard at line 225 is replaced) requires updating this test even when behavior is unchanged. The test becomes a false failure gate.
- No new coverage. The three behavior tests already exercise every correctness property the SQL-shape assertions imply. This test adds zero protection against regressions.
- Coupled churn risk. The
COUNT(*) pre-check at lines 225–230 is a known candidate for removal (see Coupled Issue below). If the prune SQL changes as a result, this test will fail for unrelated reasons, creating noise in review.
Proposed change
- Delete
retention_prune_sql_deletes_offset_tail (lines 374–396) entirely.
- If there is concern about the
OFFSET-based strategy being accidentally abandoned: add a single EXPLAIN QUERY PLAN assertion (matching the existing recent_query_uses_index test at line 497) that verifies the prune query walks idx_history_recent rather than doing a full scan — this tests the performance contract, not the SQL shape.
- No other test changes required. The three behavior tests remain the source of truth.
Guard: before deleting, confirm retention_prunes_to_max_rows_and_keeps_newest, retention_prune_helper_deletes_zero_when_under_cap, and retention_prune_helper_deletes_oldest_tail_over_cap all pass green.
Recommendation / triage
DO — but coupled with the COUNT(*) issue. If the COUNT(*) pre-check in prune_old_entries (lines 225–230) is simplified or removed, the prune SQL will change anyway, and this test will need updating regardless. Decide the count-query issue first, then drop or rewrite retention_prune_sql_deletes_offset_tail in the same PR to avoid touching this file twice.
Acceptance criteria
Traceability
clawpatch finding ID: fnd_sig-feat-ui-flow-5955f2836e-370d_b05afa5ba8
Revalidate command:
clawpatch revalidate --finding fnd_sig-feat-ui-flow-5955f2836e-370d_b05afa5ba8 --reasoning-effort high
Coupled issue: This change should be batched with the COUNT(*) pre-check decision in prune_old_entries (line 225). Resolve that issue first; land both changes in the same PR.
Problem
retention_prune_sql_deletes_offset_tail(line 374) asserts internal SQL fragments emitted byretention_prune_sql()(line 209) rather than any observable runtime outcome:The test calls
retention_prune_sql()directly and string-matches on"LIMIT -1 OFFSET ?1","WHERE id IN (","NOT IN", and"ORDER BY created_at DESC, id DESC".Any semantics-preserving rewrite of the helper (e.g. switching to a
NOT INanti-join, a CTE, or aDELETE … RETURNING-based approach) would break this test even if the three behavior tests below continue to pass.Behavior already covered by adjacent tests:
retention_prunes_to_max_rows_and_keeps_newestretention_prune_helper_deletes_zero_when_under_capretention_prune_helper_deletes_oldest_tail_over_capoverflowrows deleted; oldest survivor correctWhy it matters
COUNT(*)guard at line 225 is replaced) requires updating this test even when behavior is unchanged. The test becomes a false failure gate.COUNT(*)pre-check at lines 225–230 is a known candidate for removal (see Coupled Issue below). If the prune SQL changes as a result, this test will fail for unrelated reasons, creating noise in review.Proposed change
retention_prune_sql_deletes_offset_tail(lines 374–396) entirely.OFFSET-based strategy being accidentally abandoned: add a singleEXPLAIN QUERY PLANassertion (matching the existingrecent_query_uses_indextest at line 497) that verifies the prune query walksidx_history_recentrather than doing a full scan — this tests the performance contract, not the SQL shape.Guard: before deleting, confirm
retention_prunes_to_max_rows_and_keeps_newest,retention_prune_helper_deletes_zero_when_under_cap, andretention_prune_helper_deletes_oldest_tail_over_capall pass green.Recommendation / triage
DO — but coupled with the
COUNT(*)issue. If theCOUNT(*)pre-check inprune_old_entries(lines 225–230) is simplified or removed, the prune SQL will change anyway, and this test will need updating regardless. Decide the count-query issue first, then drop or rewriteretention_prune_sql_deletes_offset_tailin the same PR to avoid touching this file twice.Acceptance criteria
retention_prune_sql_deletes_offset_tailis removed (or replaced with anEXPLAIN QUERY PLAN-based index-use assertion).retention_prunes_to_max_rows_and_keeps_newestpasses — row count equals cap, correct oldest survivor.retention_prune_helper_deletes_zero_when_under_cappasses — zero rows deleted at- and under-cap.retention_prune_helper_deletes_oldest_tail_over_cappasses — exact overflow deleted, correct oldest survivor.recent_query_uses_indexpasses (no regression on read-path index).Traceability
clawpatch finding ID:
fnd_sig-feat-ui-flow-5955f2836e-370d_b05afa5ba8Revalidate command:
Coupled issue: This change should be batched with the
COUNT(*)pre-check decision inprune_old_entries(line 225). Resolve that issue first; land both changes in the same PR.