Repository navigation
feat(aead-value): name an FfiValue's kind without a value - #364
Conversation
A binding for a dynamically typed host has to declare what a field's values are before it has one, and the crate offered no "type without a value". cipherstash/stack#1069 had to mirror FfiValue's variants in its own eleven-variant enum, with hand-kept tables from each variant to its leaf tags, from a value to its variant, and to and from wire names. Every new FfiValue variant would have to be added there too, with nothing failing if it was missed. ValueKind is that vocabulary, owned next to the tag table it mirrors: FfiValue::kind, ValueKind::{ALL, name, tags, holds}, Display and FromStr. Null, Undefined and Passthrough have no kind: the first two are single-valued, and passthrough is a transport choice whose payload has a kind of its own. FfiValue::kind matches exhaustively, so a new variant must choose its kind before the crate compiles. The names (bool, int32, int64, uint32, uint64, float32, float64, string, bytes, array, object) are already on the wire in the consumer's plan grammar and are frozen like the tags. Conversion and index rules stay with the consumer.
🧬 Mutation testing (cargo-mutants,
|
| caught | missed | unviable | timeout |
|---|---|---|---|
| 9 | 0 | 5 | 0 |
✅ Every mutant in the changed lines was caught by a test.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The CRAP gate ran on unpinned stable (1.98.1 today) while test.yml pins 1.96.0 so that trybuild snapshots do not break on a compiler that rewords a diagnostic. The gate also runs every workspace test under coverage, trybuild included, so the vitaminc-protected snapshot protected_ref_cannot_be_constructed.stderr failed on this PR for a note line rustc 1.98 added, with no change to that crate. Pin both jobs to the same release and bump them together.
|
The red CRAP gate was a toolchain mismatch inside CI, not this change: |
✅ No CRAP threshold violations670 function(s) analyzed · threshold 30 |
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟢 merge as it is (2 of 4 review job(s) failed)
Nothing must change before merging. One optional test can go in now or in a follow-up. It checks that ValueKind::tags() agrees with the tag that the encoder writes.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 1 found, 1 posted |
| claude | claude-opus-5-5 | rust | 1 found, 0 posted |
| codex | gpt-5.6-sol | test-gap | failed |
| codex | gpt-5.6-sol | rust | failed |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. 1 comment(s) had a problem that stopped the reader acting; it rewrote 1.
Stack: not part of a stack.
Context loaded: the description, 0 linked issue(s) and 3 discussion entries.
auxesis
left a comment
auxesis
left a comment
There was a problem hiding this comment.
@coderdan thanks for this.
Approved in advance of the extra test identified by @cipherstash-bot being added.
ValueKind::tags() is a second variant-to-tag map beside the match in encrypt_with_aad. The kat_* tests pin each side to literal bytes separately, so a tag moved in one map and not the other passed every test. This pins the two maps to each other, as the review on #364 asked.
cipherstash-bot
left a comment
cipherstash-bot
left a comment
There was a problem hiding this comment.
Recommendation: 🟢 merge as it is
Nothing must change before merging. Two optional changes can go in now or in a follow-up. The first makes the new encoder-tag test fail when its list of values misses a kind. The second corrects a test comment in kind.rs that says the compiler checks more than it does.
The new code has no unsafe code and no panic sites. FfiValue::kind, ValueKind::name and ValueKind::tags use exhaustive matches with no wildcard. FromStr matches names exactly and is case-sensitive. cargo test -p vitaminc-aead-value passes on this branch.
How this review was made
| Agent | Model | Review type | Result |
|---|---|---|---|
| claude | claude-opus-5-5 | test-gap | 1 found, 1 posted |
| claude | claude-opus-5-5 | rust | 1 found, 1 posted |
| codex | gpt-5.6-terra | test-gap | 0 found, 0 posted |
| codex | gpt-5.6-terra | rust | 0 found, 0 posted |
Synthesis: claude-opus-5-5 merged the findings, removed duplicates and dropped findings it could not confirm in the code. 0 posted finding(s) were raised by two or more models.
Plain language: claude-opus-5-5 read every comment as a new reader would. No comment had a problem that stopped the reader acting, so none was rewritten.
Stack: not part of a stack.
Context loaded: the description, 1 linked issue(s) and 8 discussion entries.
| /// against literal bytes. This pins the two sides against each other, so a | ||
| /// tag moved in one and not the other fails here. | ||
| #[test] | ||
| fn every_kinded_leaf_seals_under_a_tag_its_kind_names() { |
There was a problem hiding this comment.
Optional: every_kinded_leaf_seals_under_a_tag_its_kind_names still passes when its list of values misses a kind.
Impact: The test checks only the values in its own hand-written list. That list does not come from ValueKind::ALL. Suppose a later change adds a kind with a new tag, but does not add a value to this list. Then this test does not compare the encoder with tags() for that kind, and it still passes. The test also checks in one direction only. It checks that each written tag is in kind().tags(). It does not check that the encoder writes every tag that tags() names.
Evidence: In a separate worktree, I removed the FfiValue::Bytes value from the list. The current test still passed (1 passed; 0 failed). With the fix below, the same removal makes the test fail:
left: [2, 3, 4, 5, 6, 7, 8, 9, 10]
right: [2, 3, 4, 5, 6, 7, 8, 9, 10, 11]
With all ten values in place, the changed test passes.
Fix: Collect each written tag in the loop. After the loop, compare the sorted list with every tag that ValueKind::ALL names. The tests module in value.rs does not import ValueKind, so the code uses crate::ValueKind.
#[test]
fn every_kinded_leaf_seals_under_a_tag_its_kind_names() {
let mut written = Vec::new();
for value in [
// the same ten values as now
] {
let kind = value.kind().expect("a scalar leaf has a kind");
let tag = leaf_bytes(value)[0];
assert!(
kind.tags().contains(&tag),
"{kind} does not name tag {tag:#04x}"
);
written.push(tag);
}
written.sort_unstable();
let mut named: Vec<u8> = crate::ValueKind::ALL
.iter()
.flat_map(|kind| kind.tags().iter().copied())
.collect();
named.sort_unstable();
assert_eq!(written, named, "every tag a kind names is written by the encoder");
}The changed test fails when the list misses a kind. It also fails when tags() names a tag that the encoder does not write.
Found by 1 model: claude
| /// One value per `FfiValue` variant, and the kind it should report. | ||
| /// | ||
| /// The `match` is exhaustive with no wildcard, so adding an `FfiValue` | ||
| /// variant fails to compile here until it is given a sample and a kind |
There was a problem hiding this comment.
Optional: this comment says the compiler forces a sample for each new FfiValue variant, but the compiler forces only a kind.
Impact: The match on &value has no wildcard, so a new FfiValue variant needs a new arm here. The compiler does not require a new value in the all array. It also does not require a new entry in ValueKind::ALL, because ALL is a plain array with a fixed length. FromStr searches only ALL, so a kind that is missing from ALL cannot be parsed from its name. A reader who trusts this comment may not check these two lists by hand.
Evidence: In a separate worktree, I added a ValueKind::Extra variant with arms in name() and tags(). I did not add it to ALL. The crate compiled, and cargo test -p vitaminc-aead-value reported 87 passed; 0 failed. The tests names_are_the_frozen_wire_spelling_and_round_trip and tags_partition_the_scalar_tag_table iterate over ALL, so they do not see the new kind. every_variant_reports_its_kind_and_all_covers_every_kinded_variant finds a missing ALL entry only when all has a value of that kind.
Fix: Make the comment say what the compiler checks and what it does not check. For example:
/// One value per `FfiValue` variant, and the kind it should report.
///
/// The `match` has no wildcard, so a new `FfiValue` variant fails to
/// compile here until it is given a kind. The compiler does not force a
/// sample into `all` or a new kind into `ValueKind::ALL`. Add both by hand:
/// `FromStr` searches `ALL`, so a kind missing from it cannot be parsed.
Found by 1 model: claude
3 participants
Summary
A binding for a dynamically typed host (JavaScript, PHP, Ruby) has to say what a field's values are before it has a value. This crate had no way to name "a
uint64" without holding one, so cipherstash/stack#1069 copiedFfiValue's variants into its own enum, plus hand-kept tables mapping each variant to its tags, a value to its variant, and the variant to and from a name. If a newFfiValuevariant was added, nothing would fail when those copies missed it.This PR adds
ValueKind: theFfiValuemodel without the payload, kept next to the tag table it mirrors.Changes
New module
kind(packages/aead-value/src/kind.rs), re-exported from the crate root:Null,UndefinedandPassthroughhave no kind.kind()returnsNonefor them and no kind holds them.NullandUndefinedeach have exactly one value, so declaring a field as one tells a binding nothing. Passthrough means "send this in the clear", which is a transport choice, not a type; the value inside it has its own kind. The consumer already treats a typed field as refusing null."UInt64","u64","null"are refused.ArrayandObjectmap to no tags. They are sealed as structure (the cipher's sequence and map modes), not as tagged leaves.FfiValue::kinduses an exhaustivematchwith no wildcard, so a new variant has to pick its kind before the crate compiles.Verification
Run the way
test.ymlruns it:cargo fmt -- --check: cleancargo clippy --no-deps --all-targets --all-features -- -D warnings: cleanRUSTDOCFLAGS="-D warnings" cargo doc --workspace --all-features --no-deps: cleancargo test -p vitaminc-aead-value: 86 unit tests and 1 doctest pass. The new tests check that everyFfiValuevariant reports the expected kind (an exhaustive match in the test too), thatALLlists every kind in order, thatholdsagrees withkindfor every kind/value pair, thatname()andFromStrround-trip and reject near-misses, and thattags()splits thetags.rstable exactly: every tag exceptNULL/UNDEFINEDbelongs to one kind.cargo mutants -p vitaminc-aead-value --in-diff: 14 mutants, 9 caught, 5 unviable, none missed.Related
Closes #365.
Refs cipherstash/stack#1069. Stack can delete its own copies (
of,tags,holds,name,parse,all) and keep only its index-admission and numeric-read rules. That needs a 0.5.x release ofvitaminc-aead-valuewith this in it first. This is an additive, non-breaking change.