feat(tests): add the golden decision tables, and fix two engine bugs they found
The parity contract from CLAUDE.md section 7 now exists: 42 cases across 11 scenario groups, with a runner that asserts every case and a coverage check that asserts every mandatory scenario is claimed. **A design change, and why.** ADR-16 originally specified JSON tables, justified by "serde_json will already be in the tree". That was wrong — `rusqlite` does not pull `serde` by default and `uniffi` has not landed, so nothing provided it, and `serde` is not a section 3 dependency (rule 0.3). The tables are therefore a Rust builder DSL: the same declarations, but a typo in a field name is a compile error rather than a test that silently asserts the wrong thing. For a document whose entire job is to be a contract, compile-time checking beats diffability. Converting to JSON later is mechanical if serde is approved. **Two engine bugs the tables caught, both real:** 1. **Overnight windows woke the daemon immediately.** `time_window_boundary` computed the end as `end - minute` and clamped at zero. For a 22:00–06:00 window active at 23:00, "06:00 today" is in the past, so the boundary became *now* — and the daemon would wake, re-evaluate, get the same answer, and wake again, forever. Not a missed block, but a busy loop burning the section 5 idle budget. Fixed by detecting the late half of an overnight window and crossing midnight properly. 2. **An inactive schedule woke a day early.** The next-start search iterated day offsets to test membership but computed the delta as `start - minute` only, wrapping by 1440 without advancing the day. A Monday–Friday schedule evaluated on Saturday reported Monday's start as 21 hours away — Sunday 09:00, a day not in the set. The block still fired, because the next evaluation recomputed it, but the daemon woke a day early for nothing. Fixed by folding the day offset into the delta. **Four of my own table cases were wrong too**, which is the other half of why they are worth having: a UTC+5 offset is 15:00 local and therefore *inside* 09:00–17:00 (I had asserted outside); the same for the DST cases. The engine was right and the test was wrong in all four. **Coverage bookkeeping is checked, not promised.** `coverage::Location` records whether a scenario is covered by a table or by a code fixture — clock tampering and the 24-hour cap are about the passage of time and a bound across lock levels, not a decision at an instant, so they cannot be expressed as data. Both directions fail: an uncovered scenario is an error, and so is a `covers` tag that is not on the mandatory list, which is how a case quietly stops testing what its name claims. **`BlockReason` now carries its target.** A schedule may name twenty targets and they do not all become blocked — some are paused, some belong to a condition that is not currently active, some are on the never-block list. Without carrying which target a reason is about, two blocked apps under one schedule produce two identical reasons and the UI has to re-derive the pairing, which is exactly the drift `BlockReason` exists to prevent. The tables could not assert per-app attribution without it. **The DST gap is recorded, not papered over.** `CivilTime` is an epoch plus an offset with no timezone database behind it, so the tables pin that a fixed local window stays fixed as the offset moves — including across a DST transition — but cannot express a spring-forward time that does not exist or a fall-back time that is ambiguous. Those are section 7 cases that are genuinely not covered, and LIMITATIONS.md now says so. Co-Authored-By: Claude Code <noreply@anthropic.com>
This commit is contained in:
+39
-18
@@ -214,6 +214,7 @@ pub fn evaluate(input: &EngineInput<'_>) -> Decision {
|
||||
ScheduleMode::Blocklist => {
|
||||
for target in &schedule.targets {
|
||||
let reason = BlockReason {
|
||||
target: target.clone(),
|
||||
schedule_id: schedule.id.clone(),
|
||||
schedule_name: schedule.name.clone(),
|
||||
condition: active[0],
|
||||
@@ -245,6 +246,7 @@ pub fn evaluate(input: &EngineInput<'_>) -> Decision {
|
||||
continue;
|
||||
}
|
||||
blocked_apps.push(BlockReason {
|
||||
target: Target::App(id.clone()),
|
||||
schedule_id: schedule.id.clone(),
|
||||
schedule_name: schedule.name.clone(),
|
||||
condition: active[0],
|
||||
@@ -384,38 +386,57 @@ fn bucket_reset(now: CivilTime, window: &crate::model::UsageWindow) -> CivilTime
|
||||
/// Next boundary of a time window: either its end today, or its start if the
|
||||
/// window is not currently active.
|
||||
fn time_window_boundary(window: &crate::model::TimeWindow, now: CivilTime) -> Option<CivilTime> {
|
||||
// A window with equal bounds matches nothing, ever. It must therefore
|
||||
// contribute no boundary: a timer that exists only to discover the window
|
||||
// still does not match is a wakeup spent on nothing, against the section 5
|
||||
// idle budget. The `.max(1)` above keeps a legitimately-current boundary in
|
||||
// the future; this keeps an impossible one from existing at all.
|
||||
if window.start == window.end {
|
||||
return None;
|
||||
}
|
||||
|
||||
let minute = minute_of_day(now);
|
||||
let today = weekday(now);
|
||||
|
||||
if time_window_contains(window, now) {
|
||||
// Currently active: the end depends on whether we're in the early or
|
||||
// late part of an overnight window.
|
||||
let end_today = if window.is_overnight() && minute < window.end {
|
||||
// Early part — ends later today.
|
||||
window.end
|
||||
// Currently active. Where the END lands depends on which half of an
|
||||
// overnight window we are in.
|
||||
//
|
||||
// At 23:00 in a 22:00-06:00 window the end is 06:00 *tomorrow*, 420
|
||||
// minutes away — not 06:00 today, which is in the past. Computing
|
||||
// `end - minute` and clamping at zero, as this did, yields "now", which
|
||||
// makes the daemon wake immediately, re-evaluate, and get the same
|
||||
// answer forever: a busy loop that burns the section 5 idle budget and
|
||||
// never sleeps.
|
||||
let minutes_until = if window.is_overnight() && minute >= window.start {
|
||||
// Late part: crosses midnight.
|
||||
(24 * 60) - i64::from(minute) + i64::from(window.end)
|
||||
} else {
|
||||
window.end
|
||||
// Either a daytime window, or the early part of an overnight one.
|
||||
i64::from(window.end) - i64::from(minute)
|
||||
};
|
||||
let minutes_until = i64::from(end_today) - i64::from(minute);
|
||||
return Some(CivilTime {
|
||||
epoch_minutes: now.epoch_minutes + minutes_until.max(0),
|
||||
epoch_minutes: now.epoch_minutes + minutes_until.max(1),
|
||||
utc_offset_minutes: now.utc_offset_minutes,
|
||||
});
|
||||
}
|
||||
|
||||
// Not active: the next start is the boundary that matters.
|
||||
//
|
||||
// The day offset must be folded into the delta, not just used to test
|
||||
// membership. Iterating day offsets and computing only `start - minute`
|
||||
// wraps by 1440 without advancing the day, so an inactive weekday schedule
|
||||
// on a Saturday would report Monday's start as 21 hours away — Sunday 09:00,
|
||||
// a day that is not in the set. The block still fires, because the next
|
||||
// evaluation recomputes it, but the daemon wakes a day early for nothing,
|
||||
// which is exactly what the section 5 idle budget forbids.
|
||||
let mut best: Option<i64> = None;
|
||||
for day_offset in 0..=7 {
|
||||
let candidate_day = (today as i32 + day_offset) % 7;
|
||||
if !window.days.contains(&(candidate_day as u8)) {
|
||||
for day_offset in 0..=7i64 {
|
||||
let candidate_day = ((today as i64 + day_offset) % 7) as u8;
|
||||
if !window.days.contains(&candidate_day) {
|
||||
continue;
|
||||
}
|
||||
let mut delta = i64::from(window.start) - i64::from(minute);
|
||||
if window.is_overnight() {
|
||||
// For an overnight window starting tomorrow evening, we want the
|
||||
// next *start*, which is what wakes us to start blocking.
|
||||
delta = i64::from(window.start) - i64::from(minute);
|
||||
}
|
||||
let mut delta = day_offset * 24 * 60 + i64::from(window.start) - i64::from(minute);
|
||||
if delta < 0 {
|
||||
delta += 24 * 60;
|
||||
}
|
||||
@@ -436,7 +457,7 @@ fn time_window_boundary(window: &crate::model::TimeWindow, now: CivilTime) -> Op
|
||||
/// Unlike [`condition_boundary`], which answers "when does a *firing* condition
|
||||
/// stop firing", this also covers a condition that is currently false — a time
|
||||
/// window that has not started yet still has a start, and that start is exactly
|
||||
/// when the daemon must wake up to begin blocking.
|
||||
/// when the daemon must wake to begin blocking.
|
||||
///
|
||||
/// Returns `None` when nothing about the schedule is time-dependent, which is
|
||||
/// the case for Wi-Fi and location schedules: only an OS event can change them,
|
||||
|
||||
+30
-5
@@ -123,12 +123,23 @@ pub struct Schedule {
|
||||
|
||||
/// Why a target is currently blocked, and until when.
|
||||
///
|
||||
/// Section 1.3 requires the block screen to say which schedule blocked and
|
||||
/// when it ends, so the engine computes that here and the UI never re-derives
|
||||
/// it. Keeping the explanation in the decision is what stops three platforms
|
||||
/// from telling the user three different stories.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
/// Section 1.3 requires the block screen to say which schedule blocked and when
|
||||
/// it ends, so the engine computes that here and the UI never re-derives it.
|
||||
/// Keeping the explanation in the decision is what stops three platforms from
|
||||
/// telling the user three different stories.
|
||||
///
|
||||
/// The `target` field is not decoration. A schedule may name twenty targets and
|
||||
/// they do not all become blocked: some are paused, some belong to a condition
|
||||
/// that is not currently active, and some are on the never-block list. Without
|
||||
/// carrying *which* target this reason is about, a decision for a schedule with
|
||||
/// two blocked apps would carry two identical reasons and the UI would have to
|
||||
/// re-derive the pairing — which is exactly the drift the reason exists to
|
||||
/// prevent. Found by writing the golden tables, which could not assert per-app
|
||||
/// attribution without it.
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub struct BlockReason {
|
||||
/// The specific target being blocked, not merely the schedule.
|
||||
pub target: Target,
|
||||
pub schedule_id: String,
|
||||
pub schedule_name: String,
|
||||
/// The specific condition that fired, named for display.
|
||||
@@ -138,6 +149,20 @@ pub struct BlockReason {
|
||||
pub ends_at: Option<crate::engine::CivilTime>,
|
||||
}
|
||||
|
||||
impl BlockReason {
|
||||
/// A stable, comparable key for the target this reason is about.
|
||||
///
|
||||
/// Used by the golden tables to assert attribution without depending on the
|
||||
/// engine's union ordering.
|
||||
pub fn app_key(&self) -> String {
|
||||
match &self.target {
|
||||
Target::App(id) => id.clone(),
|
||||
Target::Website(d) => format!("website:{d}"),
|
||||
Target::Keyword(k) => format!("keyword:{k}"),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// A rejection of a schedule that does not satisfy section 1.2.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub enum ValidationError {
|
||||
|
||||
Reference in New Issue
Block a user