fix(validate): surface skipped artifact files as Errors (REQ-062, P0)#305
Merged
Conversation
…nly WARNs (REQ-062)
The P0 from the cross-git investigation (FEAT-135 / REQ-062). `rivet
validate` reported `Result: PASS (0 warnings)` when artifact files
failed to parse — the skip was swallowed to a stderr `log::warn!` in
`formats::generic::import_generic_directory` and never reached the
diagnostics. A user who wrote an artifact file with the wrong shape
got a green PASS over an empty load. The textbook Cederqvist cliff:
textual success over a semantically-failed operation.
── classification: F2a (error) vs F2b (legitimate skip) ──
Not every YAML file under a `generic-yaml` source path is an artifact
file. `artifacts/bindings.yaml`, `artifacts/feature-model.yaml`, and
`artifacts/variants/*.yaml` legitimately live there. Promoting every
skip to an error would make rivet's own repo fail validate. New
`classify_skip` (rivet-core/src/formats/generic.rs) re-parses a
failed file as generic YAML and decides:
1. not valid YAML at all -> ParseError (F2a)
2. top-level mapping has an `artifacts:` -> ParseError (malformed list)
3. top-level mapping has `id` AND `type` -> ParseError (artifact
written without the `artifacts:` wrapper — the F2a reproducer)
4. anything else -> NotArtifactFile (F2b — skip)
── plumbing: non-breaking ──
`load_artifacts`'s signature is unchanged (the repo has a semver gate
on the rivet-core public API). New `load_artifacts_with_skips` returns
`(Vec<Artifact>, Vec<SkippedFile>)`; `SkipKind`/`SkippedFile` are
re-exported from the crate root. `cmd_validate` calls it, collects the
`ParseError` skips, and pushes one `artifact-parse-error` Error
diagnostic per skip into the diagnostics vec — flowing into `errors`,
the text/JSON output, and the exit code. `NotArtifactFile` skips stay
silent.
The validate `--format json` diagnostic serializer also gained a
`rule` field (it previously emitted only `severity`/`artifact_id`/
`message`) — REQ-062's acceptance requires `rule: artifact-parse-error`
to be visible to a JSON consumer.
── tests ──
rivet-core: scan_skipped_files_classifies_malformed_vs_non_artifact,
classify_skip_treats_corrupt_yaml_as_parse_error
rivet-cli: validate_surfaces_parse_error_on_malformed_artifact_file
— oracle-gated: one good + one malformed artifact file
-> exit non-zero, result FAIL, errors>=1, an
artifact-parse-error diagnostic naming the file; plus an
F2b assertion that a bindings.yaml adds no error.
Verified green: cargo build --workspace, clippy -D warnings (exit 0),
the three tests above, and test_dogfood_validate (the F2b regression
guard — rivet's own bindings/feature-model/variant files must not
error).
Implements: REQ-004
Verifies: REQ-062
Refs: FEAT-135
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. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
Wave 1 / PR-A of the cross-git investigation follow-ups (#304 →
FEAT-135). Closes the P0 finding REQ-062.
rivet validatereportedResult: PASS (0 warnings)when artifactfiles failed to parse — the skip was swallowed to a stderr
log::warn!and never reached the diagnostics. A user who wrote anartifact file with the wrong shape got a green PASS over an empty
load. The cross-git investigation called this the canonical
Cederqvist cliff; running
rivet validateon rivet's own reporeproduced it.
The F2a / F2b distinction
Not every YAML file under a
generic-yamlsource path is an artifactfile —
artifacts/bindings.yaml,feature-model.yaml, andvariants/*.yamllegitimately live there. Promoting every skip to anerror would break rivet's own
validate. Newclassify_skip:ParseError(F2a)artifacts:key (malformed list)ParseError(F2a)idandtype(artifact missing theartifacts:wrapper)ParseError(F2a)NotArtifactFile(F2b — silent skip)Non-breaking plumbing
load_artifacts's signature is unchanged (rivet-core has a semvergate). New
load_artifacts_with_skipsreturns(Vec<Artifact>, Vec<SkippedFile>).cmd_validatecollectsParseErrorskips andemits one
artifact-parse-errorError diagnostic per skip — flowinginto
errors, the text/JSON output, and the exit code. The validate--format jsondiagnostic serializer also gained arulefield(REQ-062's acceptance requires
ruleto be visible to a JSONconsumer).
Test plan
rivet-core:scan_skipped_files_classifies_malformed_vs_non_artifact,classify_skip_treats_corrupt_yaml_as_parse_errorrivet-cli:validate_surfaces_parse_error_on_malformed_artifact_file— oracle-gated: one good + one malformed artifact file → exit non-zero,result: FAIL,errors >= 1, anartifact-parse-errordiagnostic naming the file; plus an F2b assertion thatbindings.yamladds no error.test_dogfood_validate— the F2b regression guard. rivet's ownbindings.yaml/feature-model.yaml/variants/*.yamlmust not error.stats_json_counts_match_validate— still green.cargo build --workspace,cargo clippy --workspace --all-targets -- -D warnings(exit 0).REQ-062 acceptance — met
Each criterion in
artifacts/cross-git-investigation.yamlREQ-062'sAcceptance:block is exercised by the integration test above.Implements: REQ-004
Verifies: REQ-062
Refs: FEAT-135
🤖 Generated with Claude Code