op meta: propagate sub-validation errors dropped by ValidationErrors::merge_all misuse

[thedavidmeister]

Closes #173.

The defect

validator 0.16.1's ValidationErrors::merge_all(parent, field, children) keeps a child result only when that child's errors carry a ValidationErrorsKind::Struct under the key field — it is written for children the derive has already wrapped with ValidationErrors::merge(Ok(()), field, elem.validate()) (see validator_derive's wrap_if_collection). The three hand-rolled impls in crates/cli/src/meta/types/op/v1.rs passed raw BitInteger / Operand / RainString results instead, whose errors sit under their own field key ("value") with kind Field. err.remove("range") / err.remove("output") therefore found nothing and every child error was filtered out.

On main:

• BitIntegerRange(BitInteger{0}, BitInteger{16}).validate() returned Ok even though MAX_BIT_INTEGER == 15. Only the min <= max parent check was live.
• Reachable from JSON: OpMeta::try_from(br#"{"name":"add","inputs":[{"bits":[0,16]}]}"#.to_vec()) returned Ok.
• Output::validate was unconditionally Ok — its parent was the literal Ok(()) and all children were dropped, so a Computed output with an out-of-bounds range and a non-ASCII computation validated.
• OperandArgRange's per-operand children were dropped too.

The fix

Each impl now merges its children the way merge is meant to be used — one ValidationErrors::merge(result, key, child.validate()) per child, under a key distinct from the parent's own "range" key:

• BitIntegerRange → "min" / "max", on top of the existing "range" order check.
• OperandArgRange → "exact", or "min" / "max" on top of "range".
• Output → "exact", or "bits" / "computation".

Distinct keys are load-bearing, not cosmetic. ValidationErrors::add_nested panics on a non-vacant entry, so nesting a child under the same "range" key the order check already occupies would turn an out-of-order and out-of-bounds range into a panic rather than an Err. test_bit_integer_range_ends_validated pins that case (range(255, 16)) and asserts all three keys are present.

Nothing outside these three impls changes: the derive-generated nested #[validate] paths already merged correctly, and no op meta fixture in the repo is newly rejected.

QA

• Discriminating tests: test_bit_integer_range_ends_validated, test_output_computed_sub_validation_propagates, test_opmeta_input_bits_validated, test_opmeta_output_bits_validated — each fails on base. Verified by reverting only the three impl Validate bodies to origin/main in the working tree while keeping this PR's test module (confirmed byte-identical to base: the diff vs origin/main for the file started at the mod tests hunk, line 283) and running cargo test -p rain-metadata --lib meta::types::op::v1 → FAILED. 12 passed; 4 failed. With the fix in place: ok. 16 passed; 0 failed.
• Mutations applied:
  • BitIntegerRange merge(result, "max", self.1.validate()) → result — KILLED by test_bit_integer_range_ends_validated, test_output_computed_sub_validation_propagates, test_opmeta_input_bits_validated, test_opmeta_output_bits_validated (4 failed).
  • BitIntegerRange merge(result, "min", self.0.validate()) → dropped — KILLED by test_bit_integer_range_ends_validated (1 failed). This one survives an is_err()-only suite: with the order check live, min can only be out of bounds when max is too, so the max merge alone still yields Err. It is the errors().contains_key("min") assertion that kills it — hence the second commit.
  • Output merge(Ok(()), "bits", bits.validate()) → Ok(()) — KILLED by test_output_computed_sub_validation_propagates, test_opmeta_output_bits_validated (2 failed).
  • Output merge(result, "computation", computation.validate()) → result — KILLED by the same two (2 failed).
  • OperandArgRange all three per-operand merges dropped — SURVIVES, and no test claims otherwise. Operand derives Validate with no constraint on its u16, so Operand::validate() is vacuously Ok and the merges cannot be observed. This is issue BitIntegerRange/OperandArgRange/Output validation drops all sub-validation errors (ValidationErrors::merge_all misuse) #173's own P06 and it stays alive; killing it needs a constraint on Operand, which is a separate question. The merge is corrected here for the same reason the type has a Validate impl at all.
• Oracle: the type's own documented contract, not the impls under test. MAX_BIT_INTEGER's doc — "BitIntegers cannot range past the size of an Operand in bits, zero indexed" — gives 0..=15 for a u16 operand, so bits [0,16] must fail; REGEX_RAIN_STRING ("printable ASCII characters and whitespace") makes \u{2665} an invalid Computation. The merge semantics come from reading validator-0.16.1/src/types.rs (merge, merge_all, add_nested) and validator_derive-0.16.0/src/quoting.rs (wrap_if_collection), not from this file.
• Category check: the issue names three units — BitIntegerRange (P03), OperandArgRange (P06), Output (P07). All three are fixed. P03 and P07 are covered by the new tests above. P06 is covered by the fix but not by a test, for the reason stated in the mutations list and acknowledged in the issue itself ("currently unobservable because Operand derives Validate with no constraints"). The issue's triage note asks whether out-of-bounds ends should error at this layer: taken as yes, since BitInteger already declares #[validate(range(min = MIN_BIT_INTEGER, max = MAX_BIT_INTEGER))] and the doc states the bound — the impls were plainly trying to enforce it.

The three NOTE: … see rainlanguage/rain.metadata#173 comments the companion AMT PR left at these spots are removed, since the assertions they were deferring are now made.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

Next included review available in 3 minutes.

View limit details

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.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: d6d55c43-499f-439a-9cb2-026649595b42

📥 Commits

Reviewing files that changed from the base of the PR and between 45ca96c and 52a0f3a.

📒 Files selected for processing (1)

• crates/cli/src/meta/types/op/v1.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}