Name only the comparisons that ran, and the set that was compared - #16
Conversation
The carried-record sweep's refusal made three claims about work it had not done. All three are invisible to the corpus: verdicts are unchanged either way, so they live entirely in the free-form reason. 1. It named all three record-local conjuncts although check_sealed computed them under &&, which short-circuits. bad-1003 through bad-1006 fail on four different conjuncts and emitted one byte-identical string. Each conjunct is now evaluated on its own and only the failures are named. 2. It named the arming comparison even where the operand set was empty, and all() over an empty set is vacuously true, so the term decided nothing. 22 of 66 evaluations over the pinned corpus reach the site that way. The clause is now omitted rather than asserted. 3. It called the operands every carried arming record. The set is built from the records the rows resolve, a strict subset, and the phrase appears nowhere in the specification, which says every arming record the row resolves. The message now describes the set it compared and its size. Message assembly moves into sealed_sweep_reason, a pure function, because a rule this file states about its own messages needs a test in this file and the old shape was unreachable from one. src/check.rs had no unit tests. Verdicts unchanged: 250/250 at suite 5019931, spec digest 759d2383, reason parity 69/193.
Bite-tested every new test by mutating the property back. Three bite; the empty-operand-set guard does not. Deleting !arming_postures.is_empty() leaves all 21 green, because all() is already vacuously true on empty, so no input can distinguish the guard's presence. That is a structural zero and the test pins the message contract rather than the guard. Kept and documented rather than deleted or left implying coverage it does not have: it states the rule where a reader meets it, and a formulation that is not vacuously true on empty would need it without announcing so.
|
Warning Review limit reached
Next review available in: 56 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSealed-record evaluation now collects individual failed comparisons instead of one boolean. Refusal messages identify evaluated failures, row-resolved operands, absent posture values, and disagreeing arming postures. Unit tests cover clean and refusal cases. ChangesSealed-record reporting
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: ⚪ Minimal · up to This localized change refines refusal messages and adds tests without supplied evidence of a correctness, runtime, security, data, or availability regression; it is merge-ready after normal checks, with no actionable merge-blocking risk remaining. Sequence Diagram(s)sequenceDiagram
participant CoverageEvaluation
participant SealedRecordCheck
participant RefusalReason
CoverageEvaluation->>SealedRecordCheck: evaluate sealed-record comparisons
SealedRecordCheck-->>CoverageEvaluation: return individual failures
CoverageEvaluation->>RefusalReason: provide failures and posture values
RefusalReason-->>CoverageEvaluation: return detailed refusal message
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI caught this, which is the index doing its job: the record names the source digest that produced it, and the refusal-message change moved the source. Regenerated rather than re-labelled. Verdicts are untouched, 0 of 250 differing, so what moved is the reason strings and the source digest. bad-1003 through bad-1006 now emit three distinct strings where they emitted one byte-identical one; reason parity stays 69/193, since the carried-record-invalid family still collapses the three kind-keyed names the corpus distinguishes, which is a separate defect from naming comparisons that did not run. checkerCommit is a placeholder on the pre-merge SHA and carries the note saying so: it answers the GitHub API while not being reachable from a fresh clone once this squash-merges, which is the access path a reproduction uses. Repoint it to the commit on main after merge.
The carried-record sweep's refusal made three claims about work it had not done. All three are invisible to the conformance corpus: verdicts are unchanged either way, so they live entirely in the free-form reason the suite declares informative.
Found by applying astrogilda's proposed specification sentence from in-toto/attestation#570 to our own refusals: an implementation can name a condition in a refusal only where that condition was evaluated on that statement, and it cannot name a comparison whose operand set was empty.
What was wrong
check_sealedcomputed the clean-row conjuncts under&&, which short-circuits, and the caller named all three.bad-1003throughbad-1006fail on four different conjuncts and emitted one byte-identical string.all()over an empty set is vacuously true, so the arming term decided nothing. Measured: 22 of 66 evaluations over the pinned corpus reach the site with that set empty. Minimal witnessok-001-caught-intercepted-fail— one caught row, records[interception, sealed], no arming record.armingrecord the row resolves".What changed
Each conjunct is evaluated on its own and only failures are named; the arming clause is omitted where its operand set is empty; the operands are described as the records the rows resolve, with their count. Message assembly moves into
sealed_sweep_reason, a pure function, because a rule this file states about its own messages needs a test in this file and the old shape was unreachable from one.bad-1003..bad-1006now emit three distinct strings where they emitted one.Verification
5019931, spec digest759d2383verified, reason parity 69/193 — unchanged.src/check.rshad zero unit tests. Nine added; 21 pass.One guard is unforced, and says so
Deleting
!arming_postures.is_empty()leaves all 21 green, becauseall()is already vacuously true on empty, so no input can distinguish the guard's presence. A structural zero, not an empirical one. Kept and documented beside the test rather than deleted or left implying coverage it does not have.Summary by CodeRabbit