fix: report a null provider when no R was resolved, and snapshot the descriptor - #293
Merged
Conversation
`provider` names where the installation was found, so a value there when nothing was found is wrong. It also broke the descriptor's contract that every key is present and null when it does not apply, which `target` and the nullable `selected_by` fields already honor. Derive it from the same condition that decides `resolved`, so the two cannot drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Per-field assertions say nothing about the fields they do not name, so a key that silently appears or disappears goes unnoticed. That is not hypothetical: an omitted key was caught by reading a diff, not by a test. The contract that every key is present and null when it does not apply cannot be proven field by field. Snapshot the descriptor for the eight shapes it can take, alongside the existing assertions rather than replacing them: the assertions prove the wiring, the snapshots pin the shape. Machine-dependent values are normalized in the test rather than through insta's redaction feature, which would need two more insta features enabled. Normalization replaces only non-null values, so a null still reads as null; otherwise the snapshots could not distinguish a key that is present and null from one carrying a value, which is the contract at stake. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The snapshots are shared across platforms, but the pattern that found paths in a diagnostic message was not symmetric: the Windows branch let a drive letter path run past the message's trailing colon, so Windows CI would have produced "<path> TOML parse error" against a snapshot reading "<path>: TOML parse error". A message that names a path names the diagnostic's own path, so substitute that exact string instead. Recognizing paths by shape cannot be made reliable: drive letters, `~`, UNC prefixes and spaces in paths each break it differently, and only Windows CI would show it. Cover the platform-specific spellings directly, since the snapshots themselves only exercise the host's. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A diagnostic carried a message spelling its path as `~/...` while its `path` field held the absolute one, so a client could not correlate the two, and `~` is unresolvable for anyone who does not know whose home it means. It also made a snapshot platform-specific. Windows temporary directories sit under the user profile, so the two spellings diverge there and the test's substitution of the diagnostic's own path finds nothing. Unix temporary directories are not under home, the spellings coincide, and the divergence is invisible locally. Build the message with the raw path and mask when rendering for a person, substituting the diagnostic's own path. Machine-facing output keeps the raw path; the terminal warning is unchanged. Co-Authored-By: Claude Opus 5 <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
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.
arf r resolveemitted a non-nullprovidereven whenresolvedwasfalseandtargetwasnull, which is wrong becauseprovidernames where the installation was located, and it broke the descriptor's contract that every key is present and null when it does not apply. It is now derived from the same condition that decidesresolved, so the two cannot drift apart.Nothing verified that contract mechanically. Per-field assertions say nothing about the fields they do not name, so a missing or extra key goes unnoticed; one omitted key was only caught by reading a diff. The descriptor is now snapshotted for the eight shapes it can take, alongside the existing assertions rather than in place of them: the assertions prove the wiring, the snapshots pin the shape.
Machine-dependent values are normalized in the test rather than through insta's redaction feature. Normalization replaces only non-null values, so a null still reads as null, preserving the distinction the contract depends on.
Two rounds of review then found the same class of problem in how a diagnostic's path was handled, both only reproducible on Windows CI:
A pattern that located paths inside a diagnostic message was asymmetric, letting a drive-letter path run past the message's trailing colon. Recognizing paths by shape is unreliable, so the pattern is gone: a message that names a path now substitutes the diagnostic's own path string directly.
The message and the
pathfield also disagreed: the message spelled the path with~whilepathheld the absolute one, so on Windows, where temporary directories sit under the user profile, substitution found nothing. A client also could not correlate the two fields, and~is unresolvable for a client that does not know whose home it refers to. The message is now built with the raw path, and masking moved to the human-facing renderer. Terminal output is unchanged.Test plan
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test(all suites pass, 0 failed)providerfails the unresolved testarf r resolveagainst a broken config under the home directory returns the raw path in JSON while the terminal warning shows the masked one🤖 Generated with Claude Code