RFC: Structured verification results (export-json) - #4727
Conversation
Proposes an opt-in, -Z-gated --export-json <path> flag writing one machine-readable file per verification run: per-harness status, failed properties, check and cover outcomes bucketed exhaustively by status, the warnings CBMC tags as such, and the provenance needed to reproduce the run. Anchored on issue model-checking#942, which has requested exactly this document since 2022. The schema example is genuine output of the proof-of-concept implementation; the vacuity motivating case (a contradictory assume reporting SUCCESSFUL) is shown end to end.
|
sorry for spamming, but this would help a lot with automatization and it seems previous PR stalled |
feliperodri
left a comment
There was a problem hiding this comment.
Thanks, thanks, thanks! Thanks for writing this up! This is a genuinely well-constructed RFC, and I appreciate that the motivation is grounded in things that actually exist in the tree rather than hypotheticals. I checked the claims and they hold up: the TODO at kani-driver/src/cbmc_property_renderer.rs:190, update_properties_with_reach_status at cbmc_property_renderer.rs:624, the --output-format=old mocking path at call_cbmc.rs:92, the SARIF writer skipping covers and successes (sarif.rs:143 and sarif_level() at sarif.rs:233), and the kebab-case keys in list/output.rs:113. The exhaustive property bucketing, the atomic-write contract, and the "null never means a guess" discipline are all more carefully specified than most RFCs in this directory.
I'm not asking you to change the design. What I'd like resolved before merge is interface
completeness and one piece of sequencing:
- The
--sarifalternatives section needs to argue on different grounds — see my inline comment. - The enum domains (
outcome.kind,verdict,failure_kind) need to be specified, not just shown by example. schema_versionneeds a compatibility policy and stabilization criteria attached to it.- Process: #4472 still carries its own
rfc/src/rfcs/0015-json-handler.md. Let's make this one the official RFC.
The rest of my comments are smaller and can be resolved in the same pass. Once these are addressed I'm happy to see this merged as Under Review and move on to reviewing the implementation.
| Kani already emits SARIF, and SARIF is a good fit for reporting defects to code-scanning tools. It is | ||
| a poor fit for reporting *proofs*, for reasons of shape rather than quality: | ||
|
|
||
| - Kani's SARIF writer skips cover properties and emits nothing for successful ones, so **a fully | ||
| green run produces an empty `results` array**. For a prover, the all-green run is the most common | ||
| and most important case. | ||
| - SARIF's model is a finding anchored to a location. Proof, cover and vacuity semantics have no | ||
| native home in it; they would live in `properties` bags — a private schema wearing a standard | ||
| schema's clothes — and every code-scanning consumer of that same file would then see results it | ||
| does not want. | ||
| - `--sarif` must remain valid SARIF. This artifact must be free to change shape while it is `-Z` | ||
| gated. One file cannot be both. |
There was a problem hiding this comment.
This is the section I'd most like reworked, because as written it invites exactly the objection you're trying to close off...
Two of the three bullets describe our writer, not the format. Skipping covers is a choice at
kani-driver/src/sarif.rs:143, and the empty-results-on-green behaviour comes from sarif_level() at
sarif.rs:233 returning None for everything except Failure/Undetermined/Unknown. SARIF 2.1.0 itself has result.kind ∈ {pass, fail, informational, notApplicable, review, open} and property bags, so a reviewer can reasonably answer "then fix the writer" to both points. Worth noting too that --sarif is already in main and is not -Z gated, so the bar for a second machine-readable artifact is higher than it would have been a release ago.
I think the argument you want is about consumer contracts rather than expressive power:
- The same file is consumed by code-scanning tools that specifically do not want proof, cover and
vacuity rows; adding them degrades that consumer to serve a different one. --sarifis stable and must stay valid SARIF; this artifact must be free to break shape while it's
-Zgated. That's the load-bearing point and it's currently the third bullet (I'd lead with it).- Vacuity has no native SARIF representation, only a property bag, which is a private schema wearing a
standard schema's clothes (you already say this so it survives the rewrite).
Could you also add a sentence on how the two artifacts avoid drifting apart? If both are derived from
one internal result structure that's a strong answer and worth stating explicitly; if they're
independent renderers, say so and say why that's acceptable.
| "stubs": [], | ||
| "verified_stubs": [] | ||
| }, | ||
| "outcome": { "kind": "COMPLETED", "verdict": "SUCCESS" }, |
There was a problem hiding this comment.
You're admirably rigorous about the property buckets, exhaustive by CBMC status, with an explicit
other catch-all so a consumer never has to guess where an unrecognised result went (lines 219–225).
I'd like the same treatment for the enums introduced here, which currently appear only by example:
outcome.kind—"COMPLETED"here,"OUT_OF_MEMORY"referenced at line 397. What's the full set?
I'd expect it to trackExitStatus(Timeout,OutOfMemory,Other) plus the completed and
never-started cases, but a consumer shouldn't have to reverse-engineer that fromcall_cbmc.rs.outcome.verdict—"SUCCESS"here; presumably a failure counterpart, and possibly an absent/null
case when the harness didn't complete.failure_kind—"NONE"at line 175; this looks like it maps toFailedProperties, so please
enumerate it and say whether a catch-all applies.
Same question as for the buckets: is each of these closed, or does an unknown value get a catch-all? A
consumer writing an exhaustive match needs to know which.
Minor and related: the keys are snake_case but these values are SCREAMING_SNAKE. Your snake_case
rationale (lines 313–320) rests on reusing kani_metadata and cbmc_output_parser types, which
doesn't extend to these, they look like new types. Either justify the mixed convention or make it
uniform.
|
|
||
| ```json | ||
| { | ||
| "schema_version": "0.1.0", |
There was a problem hiding this comment.
There's a version field but no rules attached to it, and for an RFC whose whole subject is an
interface, that's the gap I'd most want closed. Could you add a short "Compatibility policy"
subsection covering:
- What is a minor change vs. a breaking one? (I'd expect: adding a field or a new enum variant into an
existing catch-all is minor; renaming, removing, or changing the meaning of a field is major.) - What must a consumer do to be forward-compatible? Explicitly stating "consumers must ignore unknown
fields" is worth a line, because it's the difference between us being able to extend the schema and
not. - What should a consumer do on an unknown major? Refuse to parse, presumably... then say so.
- Is
warningsinside or outside these guarantees? (See my separate comment; I think outside.)
And separately, per RFC 0006's stabilization section and the template's note that open questions must
be closed before stabilization: what has to be true before this leaves -Z? Even a three-bullet
checklist ("open questions resolved, N releases of consumer feedback, schema doc shipped or explicitly
declined") gives the next person a decision procedure instead of a judgement call.
| substantial work; the design discussion there shaped this proposal, and I would welcome | ||
| @yimingyinqwqq's review. Reviewers on that PR asked for an RFC first, for real unstable gating, for | ||
| CBMC data to come from `--json-ui` rather than scraped log text, and for a standard schema approach. | ||
| This RFC exists to settle those questions before code merges. It takes RFC number `0016` rather than |
There was a problem hiding this comment.
I appreciate the courtesy here, but let's add here as 0015. I'll take care of #4472
| - **Which other flags belong in `configuration`?** `assertion_reach_checks`, `ignore_global_asm` and | ||
| `extra_pointer_checks` are recorded because each changes what a run's results mean without changing |
There was a problem hiding this comment.
Not a blocker but... I'd rather this were answered in the RFC than shipped as an open question, because the curated-list
approach has a failure mode the RFC doesn't name: a flag added later that changes result meaning is
silently absent from files written before someone notices, and there's nothing in the artifact to
indicate the omission. That's the same class of silent failure you're arguing against in the
motivation.
The alternative I don't see discussed: record the resolved effective argument set (you already carry
enabled_unstable_features and cbmc_args, so this is an extension rather than a new idea), and keep
the curated configuration.checks block as a convenience view over it. Consumers that want a specific
toggle read the named field; consumers that need to know two runs weren't comparable can diff the
whole set.
If you'd rather stay curated, that's defensible but then please state the policy in the RFC rather
than leaving it open: any flag that changes which properties are generated, or changes what a status
means, must be recorded here. That gives future PRs a test to apply.
| This is the vacuity case made machine-readable: `outcome.verdict` is `SUCCESS` and Kani's exit code is | ||
| `0`, exactly like a harness that proved something — but `checks.unreachable` names both properties | ||
| that could not actually be exercised, so a consumer no longer has to trust the exit code alone. |
There was a problem hiding this comment.
Vacuity is the RFC's headline motivation, and the example does make it visible, but a consumer still has to infer the rule from the example. Since a harness-level triviality flag is deliberately future work (line 383, and RFC 0003 already frames the concern), I'd like the RFC to state the exact predicate a consumer should apply, e.g. along the lines of checks.total > 0 && checks.success == 0 && !checks.unreachable.is_empty() whatever you consider correct.
Two reasons this is worth the paragraph. First, if every consumer derives its own rule, they'll derive subtly different ones and we'll get bug reports about it. Second, whatever the rule is, it's invalid when configuration.checks.assertion_reach_checks is false; you explain exactly why at lines 250–256, so tying the two together closes the loop and makes the schema self-documenting on its own central claim.
| cannot be discharged by the default solver. [PR #4719](https://github.com/model-checking/kani/pull/4719), | ||
| opened independently by a CBMC maintainer, surfaces this same class of dropped-quantifier warning | ||
| prominently — corroboration that this is a real gap, not a hypothetical one. These strings are CBMC's | ||
| internal-IR pretty-printer output verbatim, can run to several kilobytes with no promised structure, |
There was a problem hiding this comment.
I'm fine with carrying these, and the "opaque, do not pattern-match" framing is the right one. Two things to add:
- State that
warningsis outside theschema_versioncompatibility guarantees. The field's presence and shape are contractual, its contents are not. Otherwise a consumer can reasonably read the version promise as covering it. - Size. Your own example is ~11.6 KB for a single warning. Multiply by the standard-library-scale runs this RFC is explicitly targeting (line 40) and the artifact can plausibly become larger than everything else in it combined, which is a problem for the CI consumers that are the whole point. Worth specifying a per-warning cap or per-harness limit with an explicit truncation marker, so a consumer can tell "no more warnings" from "we stopped recording".
| - [0011-source-coverage](rfcs/0011-source-coverage.md) | ||
| - [0012-loop-contracts](rfcs/0012-loop-contracts.md) | ||
| - [0013-list](rfcs/0013-list.md) | ||
| - [0016-export-json](rfcs/0016-export-json.md) |
There was a problem hiding this comment.
Not your doing, but visible in your diff: 0014-harness-partition never got a SUMMARY.md entry when it landed (#4228), so with your line added the book navigation reads 0013 → 0016. Could you fix
it as a drive-by here?
| ## User Experience | ||
|
|
||
| ``` | ||
| cargo kani -Z export-json --export-json results.json |
There was a problem hiding this comment.
Two small things on the surface syntax:
- Every other artifact-producing flag we have is a noun:
--sarif <PATH>,--output-into-files.--export-jsonis a verb and doesn't say what's being exported — next to--sarifon a command line, something like--results-json <PATH>reads better and ages better if we ever emit a second JSON artifact. Not a hill I'll die on, but worth one paragraph in the rationale either way. - Worth also supporting
-for stdout, or explicitly declining to; piping into a consumer without a temp file is a natural CI want, and it interacts with your atomic-write contract, so it's better settled in the RFC than discovered later. - Could you justify a dedicated
-Z export-jsonover-Z unstable-options? Both patterns exist in the tree (--coveragegates onSourceCoverage; most plain options gate onUnstableOptions), and the template's footnote implies a per-RFC ident, so I think you're fine and I'd just like the choice stated rather than implied.
| the other failures above, but never changes the run's verdict. | ||
|
|
||
| `null` always means *not measured or not applicable*, never a guess, and is always distinguishable | ||
| from `0`, `false`, and `[]`. |
There was a problem hiding this comment.
This paragraph is the best part of the RFC! 🤩 The rename-based "exists implies complete" contract, the explanation of why the up-front delete is additionally needed, and the enumeration of what absence can and cannot distinguish. I hope we can keep this level of precision when the implementation PR lands; it's exactly the reasoning that's usually missing from output-format work.
RFC 0016 (model-checking#4727) is the spec under review for this feature and covers the same ground in more detail. Keeping both would leave two competing specifications for one flag, and would make the merge order between the two PRs significant. The design discussion belongs in model-checking#4727; this branch is the implementation. Note this leaves RFC number 0015 unused. model-checking#4727 numbered itself 0016 precisely to reserve 0015 for this PR, so that choice may be worth revisiting now that the file is gone.
RFC for
--export-json <path>— an opt-in,-Z-gated flag that writes one machine-readable file describing a verification run: per-harness status, failed properties, cover and check outcomes, and the provenance needed to reproduce the run.Why. Today the only way a program can read Kani's results is to grep rendered text for
VERIFICATION:-,** N of M cover properties satisfied, andVerification Time:. Those are printing details, not an interface — when they change, a consumer's grep silently matches nothing.The specific gap: a proof can pass while proving nothing, and no machine-readable signal says so. A contradictory
kani::assumemakes every assertion unreachable; the harness reportsVERIFICATION:- SUCCESSFULand exits 0. Kani's text output says** 0 of 2 failed (2 unreachable)— a program reading the exit code sees a clean proof. This RFC's schema makes that distinction, and the vacuity ones like it, machine-readable.Why not
--sarif. SARIF is findings-shaped: Kani's writer skips cover properties and emits nothing for successful ones, so a fully green run produces an emptyresultsarray. Proof, cover and vacuity semantics would have to live inpropertiesbags — a private schema wearing a standard schema's clothes — and--sarifmust stay valid SARIF while this artifact must be free to change shape under-Z. One file cannot be both.Relation to #4472. @yimingyinqwqq proposed this capability there and did substantial work; the design discussion on that PR shaped this proposal, and I'd genuinely welcome their review. This RFC takes
0016rather than0015to leave that PR's number with it. Written RFC-first because that is what reviewers asked for on #4472.Status. Implemented as a proof of concept; the schema example in the RFC is real output. Happy to open the implementation PR alongside, or keep this standalone — whichever you prefer.
What is deliberately excluded, and why
getrusage(RUSAGE_CHILDREN)and rejected:ru_maxrssis a process-wide running maximum, so any figure is order-dependent (only a harness that out-peaks all its predecessors gets one) and meaningless under--jobs. Honest measurement needs per-child accounting (wait4()-based rusage or a per-child cgroup) — future work, sketched in the RFC. CI consumers infer OOM from exit code 137 today.-Zwith an explicitschema_versionuntil real consumers have exercised it.Open questions I'd like input on
schemarsdependency, which is not currently in the workspace — a real dependency decision rather than something to slip in.autoharnesssubcommand, or is that a follow-up once the harness-level shape settles?--coverage) — include here, or leave in their existing artifact? (Todaycode_coverageproperties are outsidechecks/coversand outsiden_properties, so the partition invariant holds; a dedicated bucket is the alternative.)configurationnow recordsassertion_reach_checks,ignore_global_asmandextra_pointer_checks— the soundness-relevant toggles we identified. What other options merit the same treatment, and should there be an explicit policy for when a flag must be recorded?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 and MIT licenses.