fix(yaml): rowan parser handles flush-left block sequences + links loud-fail (REQ-091, v0.13.1)#329
Open
avrabe wants to merge 3 commits into
Open
fix(yaml): rowan parser handles flush-left block sequences + links loud-fail (REQ-091, v0.13.1)#329avrabe wants to merge 3 commits into
avrabe wants to merge 3 commits into
Conversation
…ud-fail (REQ-091)
Two REQ-091 fixes that together close the F2 silent-failure where the
default `rivet validate` (salsa + rowan-yaml) silently graded zero
links for artifacts written in YAML's flush-left list style.
## The CST bug — flush-left sequences silently dropped
`yaml_cst::parse_mapping_entry` required `child_indent > entry_indent`
when descending into a same-line `value` of a mapping. YAML allows the
"zero-indent" block sequence form where the dashed list sits at the
same column as the parent key:
artifacts:
- id: A <- dash at column 0, same as `artifacts:`
type: req
For this shape the parser silently treated the value as empty and
dropped the entire sequence from the tree. `extract_schema_driven` then
returned 0 artifacts + 0 diagnostics, but `rivet list` (which routes
through `parse_generic_yaml`) returned the artifact correctly — the
link graph the validator should grade was invisible.
The fix: when the next-line content is a Dash, accept
`child_indent >= entry_indent`. This is YAML 1.2's zero-indent
block-sequence rule (the parser already handled `child_indent >
entry_indent` for the indented form; the equal-indent case is
necessary at the document root where there is no shallower indent to
go to).
CST probe before vs after — both shapes now produce identical trees:
`Mapping → MappingEntry → Value → Sequence → SequenceItem → Mapping`.
## The F2 silent-failure — `extract_links_via_serde` returned empty without signal
`extract_links_via_serde` (the fallback for `links:` values the CST
doesn't recognise as a block Sequence — most importantly flow-style
`links: [{type:X, target:Y}]`) silently returned `Vec::new()` when
`serde_yaml::from_str` rejected the value text. The outer cardinality
validator then graded that as "links field present but empty" — the
F2 ethos violation REQ-091 calls out.
The fix: thread `&mut Vec<ParseDiagnostic>` through `extract_links`
and `extract_links_via_serde`. On serde parse error, emit an ERROR
diagnostic naming the underlying `serde_yaml` error and the value's
span. The two call sites (`extract_section_item`,
`extract_artifact_from_item`) both have `result: &mut ParsedYamlFile`
in scope, so they pass `&mut result.diagnostics` — no surface API
change beyond these internal helpers.
## Tests
- `rivet-core/tests/req_091_flush_left_yaml.rs` — REQ-091 Acceptance
#1 + #4: drives `extract_schema_driven` directly with both fixture
shapes, asserts 1 artifact + 1 link parity. Also a structural-parity
test across multi-link inputs.
- `rivet-core/src/yaml_hir.rs::tests::extract_links_via_serde_emits_diagnostic_on_parse_error`
— REQ-091 Acceptance #3: a malformed `links:` value produces a
diagnostic naming the field, not a silent empty list.
All rivet-core tests pass (1068 lib + 83 yaml-test-suite + 2 new
integration tests). `rivet validate` on the in-tree corpus: PASS.
## What this does NOT do
Acceptance #5 — re-grading the parallel agent's sphinx-needs port
corpus against the Python reference's ~2500 warnings — requires
their `feat/import-results-sphinx-needs` branch and runs outside
this PR. Once this lands they can rebase and re-grade.
Fixes: REQ-091
Refs: REQ-051, REQ-082
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
📐 Rivet artifact deltaNo artifact changes in this PR. Code-only changes (renderer, CLI wiring, tests) don't touch the artifact graph. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ 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: 6ef067f | Previous: 89f243c | Ratio |
|---|---|---|---|
link_graph_build/10000 |
28303192 ns/iter (± 719776) |
23196972 ns/iter (± 438410) |
1.22 |
This comment was automatically generated by workflow using github-action-benchmark.
Pure cosmetic — assert_eq! macros expand onto one line where rustfmt prefers and the test's two-line assertion on artifact.id/title is joined back to one line. Refs: REQ-091
…rename REQ-091 fixed the rowan-yaml CST so the salsa validate path now sees artifacts written in flush-left list style — which is exactly what the dev → aspice migration emits via serde_yaml::to_string. The existing test `apply_rewrites_dev_to_aspice_and_validate_passes` was green only because the validator could not see the migrated artifacts at all; with REQ-091 fixed, validate correctly reports that the migrated REQ-001 / FEAT-001 have no `derives-from` / `allocated-from` links — exactly the cardinality obligations the aspice schema declares. The migration is intentionally structural-only: it renames types in place, it does not invent semantic links to system-level artifacts the dev preset doesn't ship. The right test contract is therefore that validate FAILS with exactly those two cardinality errors — documenting the migration's actual behaviour rather than hiding it. Rename the test to `apply_rewrites_dev_to_aspice_and_validate_reports_expected_link_gaps`, assert `!val.status.success()`, and pin both expected error strings (`derives-from` for the migrated sw-req, `allocated-from` for the migrated sw-arch-component). Add a comment block tying this back to the pre-REQ-091 silent-success it replaces. Refs: REQ-091 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two REQ-091 fixes that together close the F2 silent-failure where the default `rivet validate` (salsa + rowan-yaml) silently graded zero links for artifacts written in YAML's flush-left list style.
The CST bug — flush-left sequences silently dropped
`yaml_cst::parse_mapping_entry` required `child_indent > entry_indent` when descending into a same-line `value`. YAML allows the zero-indent block sequence form where the dashed list sits at the same column as the parent key:
```yaml
artifacts:
type: req
```
For this shape the parser silently treated the value as empty and dropped the entire sequence from the tree. `extract_schema_driven` then returned 0 artifacts + 0 diagnostics, while `rivet list` (which routes through the legacy `parse_generic_yaml`) returned the artifact correctly — the link graph the validator should grade was invisible.
The fix: when the next-line content is a `Dash`, accept `child_indent >= entry_indent` (YAML 1.2's zero-indent block-sequence rule).
CST probe — both shapes now produce identical trees:
`Mapping → MappingEntry → Value → Sequence → SequenceItem → Mapping`.
The F2 silent-failure — `extract_links_via_serde` silently returned empty
The fallback for `links:` values the CST doesn't recognise as a block `Sequence` (most importantly flow-style `links: [{type:X, target:Y}]`) silently returned `Vec::new()` when `serde_yaml::from_str` rejected the value text. The outer cardinality validator then graded that as "links field present but empty."
Threads `&mut Vec` through `extract_links` and `extract_links_via_serde`. On serde parse error, emit an ERROR diagnostic naming the underlying error and the value's span.
Tests
Test plan
What this does NOT do
Acceptance #5 — re-grading the parallel agent's sphinx-needs port corpus against the Python reference's ~2500 warnings — requires their `feat/import-results-sphinx-needs` branch. Once this lands they can rebase and re-grade.
Fixes: REQ-091
Refs: REQ-051, REQ-082
🤖 Generated with Claude Code