diff --git a/CHANGELOG.md b/CHANGELOG.md index 545e3946..3f678204 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,17 @@ ### Fixed +- **REQ-139 — directory import no longer warns on legitimate non-artifact + YAML.** The generic-YAML directory load warned `[WARN] skipping ` for + *every* file it declined to load — including expected non-artifact YAML + under the source path (`bindings.yaml`, `feature-model.yaml`, + `variants/*.yaml`). On a real project that put a WARN line in front of every + command, burying the signal (reported in #353). The load-path WARN now keys + off the existing REQ-062 `SkipKind`: it fires only for a genuinely malformed + *artifact* file (`ParseError`) and stays silent for `NotArtifactFile`. + `rivet validate` still re-scans and surfaces the malformed case as a hard + `artifact-parse-error` Error, so nothing is lost. Regression test. + - **REQ-115 / REQ-116 / REQ-118 — Zola export links survive a sub-directory deploy.** Artifact cross-links, document `[[ID]]` wiki-links, and the `rivet_artifact` shortcode card all emitted absolute `//artifacts/…` diff --git a/artifacts/requirements.yaml b/artifacts/requirements.yaml index e0084548..478004e6 100644 --- a/artifacts/requirements.yaml +++ b/artifacts/requirements.yaml @@ -3787,3 +3787,85 @@ artifacts: links: - type: traces-to target: REQ-115 + + - id: REQ-139 + type: requirement + title: "Directory import: only warn on malformed artifact files, stay silent for legitimate non-artifact YAML" + status: implemented + description: | + Self-reported via GitHub issue #353 part 4 (dogfooding meld). The + generic-YAML directory import warned `[WARN] skipping : …` for + EVERY file it declined to load — including legitimate non-artifact YAML + that lives under the artifacts source path (bindings.yaml, + feature-model.yaml, variants/*.yaml). On a real project that is a WARN + line on every single command, burying the signal (loud-fail register: + noise that cries wolf is as bad as silence). + + The REQ-062 `SkipKind` classification already distinguishes a malformed + artifact file (`ParseError`) from expected non-artifact YAML + (`NotArtifactFile`), but the load-path WARN ignored it. Gate the WARN + on `classify_skip(...) == ParseError`: a real problem still warns (and + `validate` still surfaces it as a hard `artifact-parse-error`); the + expected case is silent. + + Acceptance: + - `rivet list` on a project whose artifacts/ dir contains + bindings.yaml / variants/*.yaml prints no `skipping …` WARN lines. + - A genuinely malformed artifact file (e.g. `id:`+`type:` at top + level, or `artifacts:` plus an unknown sibling key) still prints a + `skipping …` WARN at load and a `artifact-parse-error` ERROR under + `rivet validate`. + tags: [dx, agent-ux, signal-to-noise, loud-fail, user-reported, issue-353] + fields: + priority: should + category: functional + baseline: v0.15.0-track + links: + - type: traces-to + target: REQ-062 + + - id: REQ-140 + type: requirement + title: "Generic-YAML load is inconsistent across commands: validate loads a file that list/get/export silently drop" + status: draft + description: | + Surfaced while verifying issue #353 against current `main` (the silent + file-skip of #353 part 1 is already fixed by REQ-062). A file with an + `artifacts:` list AND an unknown sibling top-level key (the reporter's + `traceability.yaml` with `loss-coverage:`) is handled inconsistently: + + * `rivet list` / `get` / `export` build the store via the strict + `load_artifacts` adapter (serde `deny_unknown_fields`) and DROP the + whole file — its artifacts are invisible to these commands. + * `rivet validate` builds its store via the lenient + `yaml_hir::extract_schema_driven` path, which tolerates the unknown + key and LOADS the artifacts — then ALSO reports the same file as + `artifact-parse-error` "failed to parse". + + So the same project yields different artifact sets depending on the + command, and `validate` simultaneously validates an artifact (REQ-2) + and declares its file unparseable — a contradictory signal. Verified + empirically: `rivet list` shows only REQ-1; `rivet validate` shows + REQ-2 as a loaded (mis-typed) artifact plus the parse-error. + + This is a design decision (which parser is canonical, and is the + tolerant behaviour intended?) — flagging for the maintainer rather than + unilaterally picking. The fix should make every command agree, on the + loud-fail side: a file that can't be cleanly parsed as artifacts should + be treated identically everywhere (either all load-and-warn, or all + drop-and-error), never load in one command and vanish in another. + + Acceptance: + - For a fixture file with `artifacts:` + an unknown sibling key, + `rivet list --format json` and the set `rivet validate` type-checks + contain the SAME artifact IDs. + - `rivet validate` does not both load an artifact and report its + source file as failed-to-parse. + tags: [bug, consistency, loud-fail, parser, user-reported, issue-353, needs-maintainer-decision] + fields: + priority: should + category: functional + baseline: v0.15.0-track + links: + - type: traces-to + target: REQ-062 diff --git a/rivet-core/src/formats/generic.rs b/rivet-core/src/formats/generic.rs index a5ab6bc6..560add91 100644 --- a/rivet-core/src/formats/generic.rs +++ b/rivet-core/src/formats/generic.rs @@ -223,7 +223,24 @@ fn import_generic_directory(dir: &Path) -> Result, Error> { { match import_generic_file(&path) { Ok(arts) => artifacts.extend(arts), - Err(e) => log::warn!("skipping {}: {}", path.display(), e), + Err(e) => { + // REQ-062 / #353: only a *malformed artifact file* + // (SkipKind::ParseError) is a real problem worth a + // per-command WARN. Legitimate non-artifact YAML living + // under the source path — bindings.yaml, + // feature-model.yaml, variants/*.yaml — classifies as + // NotArtifactFile and is skipped silently; warning on it + // every single command is pure noise that buries the + // signal. `rivet validate` still re-scans and surfaces the + // ParseError case as a hard `artifact-parse-error` + // diagnostic, so suppressing the noisy case loses nothing. + let is_parse_error = std::fs::read_to_string(&path) + .map(|c| classify_skip(&c) == SkipKind::ParseError) + .unwrap_or(true); // unreadable -> surface it + if is_parse_error { + log::warn!("skipping {}: {}", path.display(), e); + } + } } } else if path.is_dir() { artifacts.extend(import_generic_directory(&path)?); @@ -504,4 +521,54 @@ Artifacts: "a well-formed `artifacts:` file must produce no skip entry" ); } + + /// #353: a directory import must load the well-formed artifact files and + /// skip BOTH a legitimate non-artifact file (`bindings.yaml`, + /// NotArtifactFile — skipped silently, no WARN) and a malformed artifact + /// file (ParseError — skipped here, but surfaced by `validate`'s re-scan) + /// without erroring the whole load. This pins the load-path classification + /// that gates the per-command WARN: only ParseError warrants a WARN; the + /// NotArtifactFile case is expected and silent. + /// + /// rivet: verifies REQ-062 + #[test] + fn import_directory_loads_valid_and_skips_both_skip_kinds() { + let dir = tempfile::tempdir().unwrap(); + std::fs::write( + dir.path().join("requirements.yaml"), + "artifacts:\n - id: REQ-1\n type: requirement\n title: Valid\n", + ) + .unwrap(); + // NotArtifactFile — must be classified as such (silent skip). + let bindings = dir.path().join("bindings.yaml"); + std::fs::write(&bindings, "bindings:\n - core\n").unwrap(); + // ParseError — `artifacts:` plus an unknown sibling top-level key. + let trace = dir.path().join("traceability.yaml"); + std::fs::write( + &trace, + "loss-coverage:\n - loss: L-1\nartifacts:\n - id: REQ-2\n type: requirement\n title: Dropped\n", + ) + .unwrap(); + + let arts = import_generic_directory(dir.path()).expect("directory import must not error"); + let ids: Vec<&str> = arts.iter().map(|a| a.id.as_str()).collect(); + assert_eq!( + ids, + vec!["REQ-1"], + "only the well-formed artifact file loads; both skipped files are excluded" + ); + + // The two skipped files classify into the two distinct kinds — the + // load-path WARN suppression keys off exactly this distinction. + assert_eq!( + classify_skip(&std::fs::read_to_string(&bindings).unwrap()), + SkipKind::NotArtifactFile, + "bindings.yaml is expected non-artifact YAML — silent skip" + ); + assert_eq!( + classify_skip(&std::fs::read_to_string(&trace).unwrap()), + SkipKind::ParseError, + "an `artifacts:` file with an unknown sibling key is a malformed artifact file" + ); + } }