Skip to content

feat(config): span-annotated validation errors via miette - #110

Merged
aram-devdocs merged 2 commits into
mainfrom
codex/22-feat-config-miette-validation
Apr 25, 2026
Merged

feat(config): span-annotated validation errors via miette#110
aram-devdocs merged 2 commits into
mainfrom
codex/22-feat-config-miette-validation

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Target branch

All PRs target main. Plumb has no dev branch.

  • This PR targets main

Spec

Fixes #22

Summary

  • Add ConfigError::Validation carrying NamedSource + SourceSpan so post-deserialization issues (currently bad palette hex values) reach the user with a label that points at the offending line.
  • Wire source_code and span on figment-driven YAML/JSON parse errors via a key-search heuristic; previously they were None.
  • Walk toml::de::DeTable's spanned representation to resolve arbitrary dotted paths to byte offsets, so unknown-key, wrong-type, and bad-value diagnostics highlight the offending value precisely.
  • Validate [color.tokens] entries against #rgb, #rgba, #rrggbb, and #rrggbbaa after deserialization. The walk uses IndexMap order so the same input always surfaces the same diagnostic.

Crates touched

  • plumb-core
  • plumb-format
  • plumb-cdp
  • plumb-config
  • plumb-mcp
  • plumb-cli
  • xtask
  • docs/
  • .agents/ or .claude/
  • .github/

System impact

  • New public API item (needs doc + # Errors section if fallible)
  • New MCP tool (needs tools/list entry + protocol test)
  • New rule (needs docs page + golden test + register_builtin entry)
  • CDP / browser surface change (needs security-auditor review)
  • Config schema change (run cargo xtask schema + commit result)
  • Dependency added / bumped (cargo-deny must still pass)
  • Determinism invariant touched (see .agents/rules/determinism.md)

ConfigError::Validation is the new public variant. Its docstring covers each field; load's # Errors section was extended to mention it. No JSON Schema impact (the Config data shape is unchanged).

Architectural compliance

  • Layer discipline: plumb-core has no internal deps; unsafe only in plumb-cdp; println!/eprintln! only in plumb-cli.
  • Error shape: thiserror-derived in libs; anyhow only in plumb-cli::main.
  • No new unwrap/expect/panic! in library crates.
  • No new SystemTime::now / Instant::now in plumb-core.
  • No new HashMap in observable output paths (use IndexMap).
  • Every #[allow(...)] is local and has a one-line rationale.

The two #![allow(clippy::redundant_pub_crate)] attributes (in span.rs and validate.rs) match the workspace convention already used in plumb-core::rules::util. The #![allow(clippy::expect_used)] in the new integration test is documented in-file: integration-test helpers don't carry the #[test] proximity that clippy's allow-expect-in-tests looks for.

Test plan

  • just validate passes locally
  • cargo xtask pre-release passes (if rule or schema changed)
  • just determinism-check passes (3× byte-diff clean) — covered by just validate
  • cargo deny check passes — covered by just validate
  • New/changed behavior has a test (unit, golden snapshot, or integration)

New tests in crates/plumb-config/tests/validation_errors.rs cover the four documented failure classes plus YAML smoke tests for both Parse and Validation variants. New unit tests in validate.rs cover hex-shape acceptance/rejection. New unit tests in span.rs cover the TOML walk and the YAML key-search fallback.

Documentation

  • Rustdoc added for every new public item
  • # Errors section on every new public fallible fn
  • docs/src/ updated when user-visible behavior changed
  • docs/src/rules/<category>-<id>.md written for new rules
  • CHANGELOG updated if user-visible (otherwise release-please handles it)
  • Humanizer skill run on docs changes

No user-visible behavior outside of richer diagnostics on existing failure paths; release-please will pick this up at release time.

Breaking change?

  • No
  • Yes — describe migration path

ConfigError is #[non_exhaustive], so adding the Validation variant is non-breaking. Existing matches on Parse continue to compile and behave identically.

Checklist

  • Conventional Commits title
  • Branch name: codex/<primary>-<type>-<slug>codex/22-feat-config-miette-validation
  • All review gates passed: spec → quality → architecture → test (+ security if triggered)
  • /gh-review --local-diff main...HEAD run locally

Reviewer notes

  • Issue feat(config): [color], [radius], [alignment], [a11y] sections + schema #21 is enriching ColorSpec/RadiusSpec/AlignmentSpec/A11ySpec shapes in parallel. This PR deliberately stays out of crates/plumb-core/src/config.rs data shapes; only plumb-config (the loader) is touched.
  • TOML span recovery uses toml::de::DeTable::parse, which keeps spans on every key and value through the recursive DeValue tree. That avoids the untagged enum buffering issue that broke an earlier Spanned<Value> approach.
  • YAML and JSON span recovery is best-effort: a key-search heuristic finds the leaf segment and spans the rest of its line. The integration tests reflect this — YAML asserts source_code() is present rather than asserting on a specific span.

Surface miette-rich diagnostics on config-load failures across all
four formats (TOML, YAML, JSON) and across structural and semantic
validation paths.

- Add `ConfigError::Validation` variant carrying `NamedSource` +
  `SourceSpan` so post-deserialization issues (currently bad palette
  hex values) reach the user with a span pointing at the offending
  line.
- Populate `source_code` and `span` on figment-driven YAML/JSON parse
  errors via a key-search heuristic; previously they were `None`.
- Walk `toml::de::DeTable`'s spanned representation to resolve
  arbitrary dotted paths to byte offsets, so unknown-key and bad-value
  diagnostics highlight precisely.
- Validate `[color.tokens]` entries against `#rgb`/`#rgba`/`#rrggbb`/
  `#rrggbbaa` after deserialization. The hex check is pure (no I/O,
  uses indexmap's deterministic iteration) so the same input always
  surfaces the same diagnostic.

Tested via four failure-class integration tests plus YAML smoke tests
covering: unknown top-level key, unknown nested key, wrong type, and
bad palette value.

Fixes #22

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Apr 25, 2026

Copy link
Copy Markdown
Contributor

I have all the information I need. Let me compile the review.


PR #110 — feat(config): span-annotated validation errors via miette

Files touched: plumb-config only. No plumb-core, no MCP surface, no CDP, no schema change.


1. Determinism

cfg.color.tokens is IndexMap<String, String> (confirmed at plumb-core/src/config.rs:131), so validate::validate() iterates in insertion/deserialization order — deterministic across runs for the same input. No wall-clock, no HashMap in any output path, no env reads. Clean.

The locate_by_key_search linear scan is deterministic (returns first match). Clean.

2. Workspace layering

plumb-configplumb-core only (internally). span.rs and validate.rs have #![forbid(unsafe_code)] (inherited from lib). No println!/eprintln!. Clean.

3. Error handling

#[deny(clippy::unwrap_used, clippy::expect_used)] is active in lib.rs:10. Neither span.rs nor validate.rs contains unwrap or expect. The integration test's #![allow(clippy::expect_used)] is local and documented with a one-line rationale. thiserror throughout. Clean.

4. Test coverage

16/16 tests pass. The four documented failure classes (unknown top-level key, unknown nested key, wrong type, bad palette value) are covered for TOML with precise span assertions; YAML/JSON smoke-tested for source_code().is_some(). Unit coverage for is_valid_hex_color and the TOML walk. Clean.

5. Documentation

ConfigError::Validation has field-level docs. load()'s # Errors section names all four variants including the new one (lib.rs:104–108). Module-level doc comments on span.rs and validate.rs explain the precise-vs.-best-effort distinction. Clean.


Punch list

Warning (not blocker) — double file read in YAML/JSON paths

load_yaml (lib.rs:188–198) and load_json (lib.rs:201–211) both call fs::read_to_string(path) and then pass path to figment, which re-reads the file internally. In the error path, build_figment_parse_error annotates spans against contents (our read) while figment's error came from its own read. If the file is mutated between the two reads, the diagnostic text and span are consistent with each other (both derive from contents) but the error message may name a field that no longer exists in the on-disk content. This is benign for a CLI config loader (the user sees a coherent diagnostic regardless) but is structurally inconsistent. The TOML path avoids this entirely by parsing the string in-memory.

If this is ever tightened, the fix is to use figment's string-based providers (Toml::string, Yaml::string, Json::string) with the already-read contents rather than re-reading via Yaml::file. Not a blocker — the current behavior is correct under all realistic conditions.

Warning — validate() returns only the first error

validate.rs:27 returns Option<ValidationIssue>. If color.tokens has three bad values, the user must fix one at a time. The comment documents this explicitly. Not a blocker — the PR description acknowledges it and the behavior matches # Errors. Worth noting for follow-on work.

Observation — DeTable/DeValue in toml 1.x

span.rs:14 imports toml::de::{DeTable, DeValue}. These are public, stable API in the toml 1.x line (workspace pins toml = "1", lock resolves to 1.1.2). Build confirms they're accessible. No concern.


Verdict: APPROVE

Addresses Claude code-review feedback on PR #110. The CLAUDE.md rule is
"Never write multi-line comment blocks — one short line max" and both
`#[allow(...)]` rationales fit on one line.
@aram-devdocs
aram-devdocs merged commit 6ea352d into main Apr 25, 2026
14 of 15 checks passed
@aram-devdocs
aram-devdocs deleted the codex/22-feat-config-miette-validation branch April 25, 2026 12:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(config): span-annotated validation errors via miette

1 participant