Skip to content

zone_assets: collision()'s _ => None wildcard can make usable_collision's documented-unreachable arm reachable (#803 follow-up) #826

Description

@djhenry

Split out of the round-2 review of #821 (fix for #803). Both are non-blocking — the reviewer
said it would merge over both, and #821 was merged as 223ac9b.

1. collision()'s wildcard is the seam that can make a documented-unreachable arm reachable

usable_collision() ends with

state.collision().ok_or(NotUsable::Idle)

and its comment says that arm is unreachable, because usability returns None only for Ready
and Ready's collision field is not an Option. That is true today and it is pinned today:
usable_collision_agrees_with_usability_for_every_state enumerates all five current state variants
against five zone spellings, and forcing the fallback reachable turns ten tests red.

But the two functions disagree about how they will react to a sixth variant:

pub fn collision(&self) -> Option<&Arc<Collision>> {
    match self { Self::Ready { collision, .. } => Some(collision), _ => None }
}

collision() has a _ => None wildcard. usability matches without one. So a new state that
carries a usable grid would make usability fail to compile — the desired outcome — while
collision() silently returns None and the "unreachable" arm starts firing, turning a usable grid
into a refusal.

The pin catches this only if someone remembers to add the new variant to the test's states vec.
The compiler should be the thing that catches it. Replacing the wildcard with explicit arms makes
the two functions fail together.

Worth doing because the whole point of #803 was to stop a refusal and an answer from being
confusable, and this is the one remaining place where adding a state quietly converts one into the
other.

2. Write down why the case-sensitivity asymmetry is safe

usability compares zone names with eq_ignore_ascii_case; zone_needs_reload compares them
exactly. The round-2 review ruled this safe and the reasoning is worth keeping in-tree, because it
is not obvious and it runs the opposite way to intuition:

zone_needs_reload is strictly more eager than usability, so a case difference can only
cause a spurious reloadPending, then an honest 503. It cannot produce a stale-but-blessed
grid.

Without that sentence the asymmetry reads as an oversight, and the tempting "fix" is to make
zone_needs_reload case-insensitive too — which removes the eagerness that makes the current
arrangement safe. One comment at the comparison site.

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent-honestyClient reports success or state that is false — a lie an agent cannot detect

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions