Withhold WP timeout verdict the run didn't measure - #20
Merged
Conversation
Two things can make a run say less than its payload appears to, and neither was visible in it. A prover budget is wall clock, so on an oversubscribed host every goal grinds to it whatever its difficulty, and a timeout stops saying anything about the goal. And WP's cache defaults to update, so it replays timeout verdicts as readily as valid ones: a run whose failures all came back from the cache attempted nothing and looked identical to one that did. run_wp now reads the host load before scheduling any proof, counts how many of its timed-out goals were replayed, and withdraws the confident verdict for either, naming both causes when both apply. The reading is taken before rather than after because a timeout-heavy run drives the one-minute average toward saturation on its own, and a reading taken at response assembly is mostly this server's own provers. The load reaches the payload as a category and never as a number. run_wp hashes the timeout triage into the proof receipt, so a continuous environment-dependent float there would give two byte-identical runs on the same machine different digests, which is the one property a receipt exists to carry. Rounding does not save it, since 0.42 and 0.55 differ however few decimals they are printed to. The reading is reported beside the response, where nothing hashes it. Verification profiles gain isystem_paths, nostdinc and rte, and all three join the load identity a receipt is hashed over, because each decides which declarations a file is compiled against or which obligations exist at all. A profile a run or a conclusion will name is refused without rte and nostdinc: otherwise a receipt is checked against an invented default and passes for whichever setting happened to match it. check resolves rte itself rather than leaving it to reload_project, whose fallback of false is right for a load and wrong here, since naming a target used to turn RTE off and quietly shrink the obligation set check reports against. -nostdinc reaches Frama-C's preprocessor and not the C compiler behind E-ACSL. The modeled libc exists to be analysed rather than compiled, so dropping the real system headers from the gcc that builds the instrumented program leaves it without stdio, and a project that loads cleanly then fails to build. run_measurement asks the goal's own verdict for whether it timed out. It asked the consolidated one, which is the verdict of the property the goal hangs off, on the line after asking the goal's own verdict for whether it was proved. A timeout under a property that consolidated valid answered "not timed out", which is how a replayed timeout hides behind a confident verdict.
Three of these are holes a verdict could pass through, and two of the three were opened by the change they sit in. A profile that names no system include directories now matches only a load that passes none. It briefly matched any load at all, for symmetry with nostdinc, and that was the wrong symmetry: rte and nostdinc are Option and can say "unset", a Vec cannot, and the evidence gate requires those two flags but not this list. A profile omitting it would have matched a load carrying any -isystem, and run_wp would have labelled that run as the target's evidence under a load identity that is not the target's. The three list fields beside it have always compared exactly. check reports RTE_DISABLED for a load a profile turned RTE off on. The resolved value reached reload_project and the caller's unresolved one still reached the gap list, so naming a target produced a load with no runtime-error obligations and nothing saying so. A property Frama-C discharged only under a hypothesis that cannot hold is now reported as PROPERTY_VACUOUS. It matched neither of the two states proved_goal_gap tested for, since property_is_dead reads "_but_dead" and goal_is_valid_under_hypotheses reads "valid_under_hyp", so the goal produced no gap and check read "proved" over a proof that holds over no execution. get_wp_goals already reported it through goal_needs_failure_classification, so the two paths disagreed about the same goal. The v2 schema allows a new code without a bump. The test harness picks the fresher of the release build and the binary Cargo built for the run. Preferring release whenever the file existed meant a stale one won, and the suite then answered about the code as it was before the change under test, which is the trap CLAUDE.md records for McpHandle::spawn. Release still wins when it is newer, since it is what the timing-sensitive lifecycle tests were written against. The refusal for a profile silent on rte or nostdinc names both fields, because the guard fires when either is unset. The playbook gives each of the two its own reason, since rte decides whether obligations exist and nostdinc decides which program they are about. All 16 gates pass.
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/mcp/checkgaps.rs">
<violation number="1" location="src/mcp/checkgaps.rs:515">
P2: For ordered-instance vacuity, `proved_goal_gap` reads `vacuity_reason` and `vacuity_dependency` from the goal, but enrichment leaves those fields only on the property row. Every `PROPERTY_VACUOUS` entry from that path therefore loses the explanation and dependency; copy the fields during goal enrichment or otherwise source them from the matched property.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| "property_status": field("normalized_property_status"), | ||
|
|
||
| // Why Frama-C called it vacuous, when the property row says. | ||
| "vacuity_reason": field("vacuity_reason"), |
There was a problem hiding this comment.
P2: For ordered-instance vacuity, proved_goal_gap reads vacuity_reason and vacuity_dependency from the goal, but enrichment leaves those fields only on the property row. Every PROPERTY_VACUOUS entry from that path therefore loses the explanation and dependency; copy the fields during goal enrichment or otherwise source them from the matched property.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/mcp/checkgaps.rs, line 515:
<comment>For ordered-instance vacuity, `proved_goal_gap` reads `vacuity_reason` and `vacuity_dependency` from the goal, but enrichment leaves those fields only on the property row. Every `PROPERTY_VACUOUS` entry from that path therefore loses the explanation and dependency; copy the fields during goal enrichment or otherwise source them from the matched property.</comment>
<file context>
@@ -479,6 +489,33 @@ fn proved_goal_gap(goal: &serde_json::Value, status: &str) -> Option<serde_json:
+ "property_status": field("normalized_property_status"),
+
+ // Why Frama-C called it vacuous, when the property row says.
+ "vacuity_reason": field("vacuity_reason"),
+ "vacuity_dependency": field("vacuity_dependency"),
+ }));
</file context>
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.
Two things can make a run say less than its payload appears to, and neither was visible in it. A prover budget is wall clock, so on an oversubscribed host every goal grinds to it whatever its difficulty, and a timeout stops saying anything about the goal. And WP's cache defaults to update, so it replays timeout verdicts as readily as valid ones: a run whose failures all came back from the cache attempted nothing and looked identical to one that did.
run_wp now reads the host load before scheduling any proof, counts how many of its timed-out goals were replayed, and withdraws the confident verdict for either, naming both causes when both apply. The reading is taken before rather than after because a timeout-heavy run drives the one-minute average toward saturation on its own, and a reading taken at response assembly is mostly this server's own provers.
The load reaches the payload as a category and never as a number. run_wp hashes the timeout triage into the proof receipt, so a continuous environment-dependent float there would give two byte-identical runs on the same machine different digests, which is the one property a receipt exists to carry. Rounding does not save it, since 0.42 and 0.55 differ however few decimals they are printed to. The reading is reported beside the response, where nothing hashes it.
Verification profiles gain isystem_paths, nostdinc and rte, and all three join the load identity a receipt is hashed over, because each decides which declarations a file is compiled against or which obligations exist at all. A profile a run or a conclusion will name is refused without rte and nostdinc: otherwise a receipt is checked against an invented default and passes for whichever setting happened to match it. check resolves rte itself rather than leaving it to reload_project, whose fallback of false is right for a load and wrong here, since naming a target used to turn RTE off and quietly shrink the obligation set check reports against.
-nostdinc reaches Frama-C's preprocessor and not the C compiler behind E-ACSL. The modeled libc exists to be analysed rather than compiled, so dropping the real system headers from the gcc that builds the instrumented program leaves it without stdio, and a project that loads cleanly then fails to build.
run_measurement asks the goal's own verdict for whether it timed out. It asked the consolidated one, which is the verdict of the property the goal hangs off, on the line after asking the goal's own verdict for whether it was proved. A timeout under a property that consolidated valid answered "not timed out", which is how a replayed timeout hides behind a confident verdict.
Summary by cubic
Withholds WP timeout verdicts when the run didn't measure the goal, and closes three gaps a verdict could pass through.
Host load and cache
run_wpreads host load before scheduling proofs and reports it as a category beside the response, not in the receipt.run_measurementnow reads the goal's own verdict for timeout instead of the consolidated property verdict.Profiles and receipts
isystem_paths,nostdinc, andrte, all hashed into the proof receipt load identity; a profile naming noisystem_pathsmatches only a load passing none.rteandnostdinc; otherwise it is refused.checkresolvesrteitself rather thanreload_project's fallback, reportsRTE_DISABLEDwhen the profile turned RTE off, and reportsPROPERTY_VACUOUSfor goals proved only under an impossible hypothesis.-nostdincgoes to Frama-C's preprocessor only; the E-ACSL compiler keeps system headers.analysis.rsintocheckgaps.rsunchanged, and the test harness now picks the fresher of the release build and Cargo's build.Written for commit 8cb6a8c. Summary will update on new commits.