Repository navigation
fix(latency): a declared range budget is read, under its own property set, and a breach fails the run - #487
Merged
Merged
Conversation
… set, and a breach fails the run #455 item (2), partially — and the requirement stays `implemented`, not `verified`, because two of its clauses did not ship. Said up front so this is not read as closing the item. THE COMPARISON ALREADY EXISTED. latency.rs had it. Four independent defects kept it from ever firing on a budget anyone would write; this fixes three. (a) SCALAR ONLY. `get_timing_property` calls `extract_time_ps`, which returns None for a range. The standard types `Latency` as `Time_Range` and a range is how a budget is written — #455's own `300 ns .. 250 us` read as "no budget". (d) WRONG PROPERTY SET — found while fixing (a), not in the original triage. `Latency` is a `Communication_Properties` property (standard_properties.rs). The lookup searched `Timing_Properties`, then the empty set. So the correctly qualified spelling was the ONE spelling guaranteed never to be found; only a bare `Latency` reached it, through the empty-set fallback. `Timing_Properties` is still consulted, second, because existing models and tests here were written against the old lookup. (c) A WARNING. Even the single firing combination exited 0, so a CI gate reading the exit code saw success at 80x over budget. Now an Error. That is a DELIBERATE BEHAVIOUR CHANGE for models over budget today — release notes. It is the same defect REQ-CLI-ANALYZE-EXITCODE-001 fixed one layer down: a budget nobody can gate on. END TO END, through the real CLI, published v0.45.0 against this change, two devices summing 80 ms against a 1 ms budget: range, bare `Latency` exit 0 -> exit 1 [ERROR] exceeds bound range, Communication_Properties exit 0 -> exit 1 [ERROR] scalar (previously only a Warning) exit 0 -> exit 1 [ERROR] range budget that is MET exit 0 -> exit 0 no budget at all exit 0 -> exit 0 NOT SHIPPED (b): the flow's OWN `{ Latency => .. }`. End-to-end flow items carry `property_associations` in the item tree, and `EndToEndFlowInstance` has no properties field — they are dropped at instantiation. That is a HIR change and a separate PR. NOT SHIPPED, DELIBERATELY: the below-MINIMUM check. `best_case_ps` is not a best case. It sums `get_execution_time`, documented as the WORST-case execution time — which is why two 20..40 ms devices report `[80.000 ms .. 80.000 ms]` instead of 40..80. A minimum check over that figure compares an inflated lower bound and stays SILENT on real violations whenever the inflated sum clears the minimum: unsound in exactly the direction #455's kill-criterion needs. Filed as REQ-ANALYSIS-LATENCY-BEST-CASE-001 — the same provenance defect class this campaign keeps finding, a printed figure labelled as something it is not. A test pins that no below-minimum diagnostic ships until it is fixed. A scalar is read as an UPPER BOUND ONLY, never `min == max`. Otherwise the moment a minimum check exists, every within-budget flow breaches it. TWO EXISTING TESTS WENT VACUOUS when severity moved, and that was demonstrated rather than assumed. `latency_exactly_at_bound_no_warning` and `latency_within_bound_no_warning` filtered on `Severity::Warning` and asserted empty, so a spurious ERROR passed them. Proof: `>` -> `>=` SURVIVED the at-bound test before the fix, and is caught by it after. Negative tests now match the message at any severity; positive ones require Error. MUTANTS, each confirmed SURVIVED against the suite before its case was written: range string-path returns None only a_range_budget_is_read_and_... Communication_Properties dropped only a_budget_under_its_standard_... range max reads the min the met/breached range pair severity back to Warning three positive tests `>` -> `>=` only the at-bound test (after the fix) scalar sets min == max survived the WHOLE analysis suite — nothing consumes min_ps yet — so pinned at the accessor, where it is caught by exactly one Verified: cargo test --workspace exit 0, 3238 passed across 102 result lines, zero failures; spar-analysis 941; clippy -D warnings clean; fmt clean; workspace builds all targets; rivet validate PASS. Refs #455 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BnBWxgVEUPdsBTLNKtrA6Z
Rivet verification gate✅ 20/20 passed
Filter: Failed artifacts(none) Updated automatically by |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#455 item (2), partially —
REQ-ANALYSIS-LATENCY-BUDGET-001goes toimplemented, notverified, because two of its clauses didn't ship. Said first so this isn't read as closing the item.The comparison already existed — four defects kept it from firing
get_timing_propertyis scalar-only; the standard typesLatencyasTime_Range, so #455's own300 ns .. 250 usread as no budgetLatencyisCommunication_Properties; the lookup searchedTiming_Propertiesthen"", so the correctly qualified spelling was the one never found{ Latency => … }End to end, through the real CLI
Published v0.45.0 against this branch; two devices summing 80 ms against a 1 ms budget:
Latency[ERROR] … exceeds boundCommunication_Properties::LatencyA breach is now an Error, so models that are over budget today — and were told so only at Warning — will start failing. That is the intent: a budget nobody can gate on is the defect
REQ-CLI-ANALYZE-EXITCODE-001fixed one layer down. Needs a release-notes line.Deliberately NOT shipped: the below-minimum check
best_case_psis not a best case. It sumsget_execution_time, whose doc says "worst-case from range" — which is why two20 ms .. 40 msdevices report[80.000 ms .. 80.000 ms]rather than40 .. 80.A minimum check over that figure compares an inflated lower bound, so it stays silent on real violations whenever the inflated sum clears the minimum — unsound in exactly the direction #455's kill-criterion needs. Filed as
REQ-ANALYSIS-LATENCY-BEST-CASE-001(the provenance class again: a printed figure labelled as something it isn't). A test pins that no below-minimum diagnostic ships until it's fixed.Relatedly, a scalar budget is read as an upper bound only, never
min == max— otherwise the moment a minimum check exists, every within-budget flow breaches it.Two existing tests went vacuous — demonstrated, not assumed
latency_exactly_at_bound_no_warningandlatency_within_bound_no_warningfiltered onSeverity::Warningand asserted empty — so once the breach became an Error, a spurious Error passed them. Proof:>→>=survived the at-bound test before the fix, and is caught by it after. Negative tests now match at any severity; positive ones require Error.Mutation evidence
Each mutant confirmed surviving the suite before its case was written:
None(the original bug)a_range_budget_is_read_and_a_breach_is_an_errorCommunication_Propertiesdroppeda_budget_under_its_standard_property_set_is_found>→>=min == maxmin_psyet) — pinned at the accessor, caught by exactly onecargo mutants --in-difflocally: 10 mutants, 7 caught, 3 unviable, 0 missed,end_timepopulated. The block this replaces carried no survivors in main's last full run (38500597645), so the full-crate ratchet is expected to hold at 292 — a prediction backed by that artifact, not by reasoning alone.Verified
cargo test --workspaceexit 0 — 3238 passed across 102 result lines, zero failures ·spar-analysis941 · clippy-D warningsclean · fmt clean · workspace builds all targets ·rivet validatePASS.Refs #455
🤖 Generated with Claude Code
https://claude.ai/code/session_01BnBWxgVEUPdsBTLNKtrA6Z