fix(yaml-hir): preserve plain-scalar field values that contain commas (#747)

[avrabe]

Closes #747.

Root cause

The CST lexer in yaml_cst.rs::lex_plain_scalar intentionally breaks
plain scalars at ,, ], } (line 401). So a value like

test-name: tests::boolean_matches_aadl_boolean, tests::boolean_mismatches_aadl_string

lands in the Value node as three sibling tokens:
PlainScalar("tests::boolean_matches_aadl_boolean") / , +
whitespace / PlainScalar("tests::boolean_mismatches_aadl_string").

scalar_text() already handles this — it walks
next_sibling_or_token() until the next Newline / Comment. But
node_to_yaml_value() — the path taken for the fields.* mapping
and for unknown top-level keys — read only the first plain-scalar
token via descendants_with_tokens() and returned it verbatim. The
tail of the value was silently discarded, and the truncated result
rendered identically to a value that had only ever claimed one
element. Every comma-containing test-name in spar lost its
second entry; stpa-yaml never disagreed with generic-yaml on any
scalar that happened not to contain a comma, which is why it went
unnoticed.

Fix

In node_to_yaml_value's plain-scalar branch, reassemble the value
via scalar_text(value_node) before running type inference through
plain_scalar_to_value. Quoted scalars are unaffected — they arrive
as a single token and are still routed through
scalar_to_yaml_value unchanged.

The parser's inline-scalar branch already bumps every token up to
the newline as a direct child of Value (see parse_mapping_entry
in yaml_cst.rs), so scalar_text's sibling walk sees the whole
run.

Acceptance criteria (from the issue's "Expected" section)

• Preserve the scalar verbatim (generic-yaml behaviour).
  A comma-containing plain scalar round-trips through
  stpa-yaml unchanged. Reproducer from the issue —
  test-name: tests::first, tests::second — now returns
  "tests::first, tests::second" instead of "tests::first"
  (regression test
  schema_driven_preserves_field_value_with_commas).
• No silent one-of-N truncation. The bug hit the
  fields.* path and the unknown-top-level-key
  fallthrough (both delegate to node_to_yaml_value). Both are
  covered by regression tests
  (schema_driven_preserves_field_value_with_commas,
  schema_driven_preserves_unknown_key_value_with_commas), so
  the two paths can't regress independently.

The alternative reading in the issue — "if comma-splitting into
list is intended for this field, emit all elements and declare
the field's type as a list" — is not adopted: the schema-driven
parser has no way to know a dev-schema custom field like
test-name should be list-typed, and doing so would break every
existing scalar consumer. Preserving the scalar matches
generic-yaml, matches the on-disk YAML, and is the only outcome
that's right under both readings the issue proposes.

Test plan

• cargo test -p rivet-core --lib yaml_hir::tests::schema_driven_preserves
  — 2/2 new tests pass
• cargo test -p rivet-core --lib — 1184/1184 pass (no regressions)
• cargo test -p rivet-core --tests — integration + proptest + YAML
  test-suite (83 in the last block) pass
• cargo fmt --all --check clean
• cargo clippy -p rivet-core --lib -- -D warnings clean (only
  the pre-existing MSRV note from clippy.toml)
• cargo run -p rivet-cli --release -- validate — PASS (592
  warnings; no schema/artifact changes in this PR)

Follow-ups (out of scope)

The issue's "Note on how it was found" mentions that
rivet list --format json (without --full) omits fields, so an
A/B on that output missed this class of change. That's a real sharp
edge but it's a separate CLI-output design decision — worth its own
issue if you want it tracked.

---

Generated by Claude Code

[github-actions]

📐 Rivet artifact delta

No artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph.

[github-actions]

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rivet Criterion Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite	Current: a22db62	Previous: 3d03ee3	Ratio
validate/10000	1433211822 ns/iter (± 16151187)	1053140348 ns/iter (± 17884749)	1.36
traceability_matrix/1000	62438 ns/iter (± 849)	44614 ns/iter (± 201)	1.40
diff/10000	11918925 ns/iter (± 1730861)	9171430 ns/iter (± 402956)	1.30
query/10000	341921 ns/iter (± 3812)	226741 ns/iter (± 1748)	1.51

This comment was automatically generated by workflow using github-action-benchmark.

[avrabe]

The perf-alert is picking up something real on the runner, but it can't be caused by this diff. Reasoning:

The four benchmarks that regressed — validate/10000, traceability_matrix/1000, diff/10000, query/10000 — all build their Store in memory via generate_artifacts() (see rivet-core/benches/core_benchmarks.rs:78) and then measure validate::validate, matrix::compute_matrix, ArtifactDiff::compute, query::execute respectively. None of those code paths touch rivet_core::yaml_hir — that module is only reached from import_with_schema in lib.rs:494/522 and db.rs:169, which are the on-disk stpa-yaml load path.

grep -n 'yaml_hir\|load_artifacts\|extract_schema_driven\|import_with_schema' rivet-core/benches/core_benchmarks.rs returns zero hits. This diff modifies exactly one function (node_to_yaml_value in yaml_hir.rs), which is unreachable from the four regressed benchmarks.

The variance the alert reports (± 16151187 on 1.4B ns ≈ ±1.1%) is also inconsistent with a 30–51% real regression — a genuine change of that size would show a much wider std-dev on top. What it looks like instead is a loaded self-hosted runner (which #567 / #509 describe as recurrent). I'd trust a re-run before treating this as a blocker; happy to push a no-op commit to trigger one, or wait if a rerun is easier from your side.

The other CI gates from my end are:

• cargo test -p rivet-core --lib — 1184/1184 pass
• cargo test -p rivet-core --tests — integration + proptest + YAML test-suite pass
• cargo fmt --all --check clean
• cargo clippy -p rivet-core --lib -- -D warnings clean
• cargo run -p rivet-cli --release -- validate — PASS

---

Generated by Claude Code

[avrabe]

Kani check failed on runner shutdown, not on a proof result. Tail of the job log:

20:53:02.8136824Z    Compiling rivet-core v0.30.0
20:55:11.2336817Z ##[error]The runner has received a shutdown signal. This can happen when
                            the runner service is stopped, or a manually started runner is canceled.
20:55:11.2337211Z ##[error]Process completed with exit code 143.

Exit 143 is SIGTERM — the self-hosted runner disappeared while rivet-core was still compiling in cargo-kani -p rivet-core --only-codegen. Kani never got to run a single proof. This is the same infra class as #509 (runner pool goes offline mid-job) and #567 (post-job cleanup frees nothing), not a signal from Kani about anything in this diff.

Nothing to fix in the patch. A re-run should be green (or fail at the actual codegen step, which would then be a real signal). Same offer as on the bench alert: happy to push a no-op commit to trigger the rerun, or wait if it's easier from your side.

---

Generated by Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!