Repository navigation
feat!: keep error causes in domain_linkage, linked_vp, methods and keys - #128
Conversation
The controller of an origin maintains the DID configuration it publishes: it renews credentials before they expire and unlinks an origin by dropping its credentials. Both need each entry's DID, origin and expiry, read back from credentials it signed itself. `DomainLinkageConfiguration::entries` reads each JWT entry without verifying it, as an `UnverifiedLinkage` (DID, normalized origin, issuer, `nbf`, `exp`) or the reason it does not read. `earliest_expiry` says when renewal is due, and `remove_origin` drops the entries of an origin, compared normalized, keeping entries that do not read. `UnverifiedLinkage::from_jwt` reads one credential. The subject DID and origin are read by the same helpers `DomainLinkageValidator` uses, so both agree on what an entry says. Closes #123. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PB1rHNJ3YpjogEUJQYccc2
…and `keys` #118 made the `vc`, `sd_jwt_vc` and `jose` errors keep their cause as `source()`. These modules still formatted a wrapped error into a `String`; each now keeps it, boxed since the causes are `url`, `p256` and `k256` errors or a mix of this crate's: - `DomainLinkageError::InvalidOrigin` becomes `{ origin, reason, source }` and `InvalidService` gains `source` (the `InvalidOrigin`, URI or URL error); - `LinkedVpError::InvalidService` gains `source`, and `InvalidBody` gains `source` (the UTF-8 error or `JwsError`) with a `&'static str` reason that keeps the enum under clippy's large-error limit; - `ProduceError::UnsupportedKey` gains `source` and `InvalidParams` becomes `{ reason, source }`; `did:jwk`, `did:key` and `did:web` set it; - `KeyStoreError::InvalidKey` and `InvalidKeySet` gain `source`; the PKCS #11 store keeps the `JwkError` of a public key, the software store the key errors and the JSON error of its file. `ProduceError::unsupported_key`, `ProduceError::invalid_params` and `KeyStoreError::invalid_key` build the variants without a cause, for `DidProducer` and `KeyStore` implementations outside the crate. Every message still ends with its cause's. Closes #124. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PB1rHNJ3YpjogEUJQYccc2
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 50 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Domain-linkage and linked-VP errors src/domain_linkage/mod.rs, src/linked_vp/mod.rs, tests/domain_linkage.rs, tests/linked_vp.rs |
These errors retain optional sources for parsing and validation failures. Tests check structured error details and source types. |
Key-store validation errors src/keys/error.rs, src/keys/local.rs, src/keys/pkcs11.rs, src/keys/signer.rs, src/keys/software.rs, tests/keys.rs, tests/keys_pkcs11.rs |
Key-store errors retain causes from key conversion, thumbprint, and JSON parsing. A constructor creates cause-free invalid-key errors. Tests check source types and cause-free cases. |
DID producer errors src/resolver/producer.rs, src/methods/jwk.rs, src/methods/key.rs, src/methods/web/mod.rs, tests/methods_jwk.rs, tests/methods_key.rs, tests/methods_web.rs, tests/resolver.rs, docs/DESIGN.md |
Producer errors add optional sources and cause-free constructors. DID method adapters and resolver tests preserve and inspect causes. The design documentation describes the error behavior. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix
Possibly related PRs
- impierce/identity-kit#118: Establishes the
source()and cause-display convention that this PR applies to additional modules. - impierce/identity-kit#120: Updates the
causehelper used by this PR to format optional sources.
Merge Risk: 
b0c10
-
No actionable merge-blocking issue is established; proceed with the normal test checks before merging.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 60.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 56 functions across 19 files. (1 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly summarizes the primary change: preserving error causes across domain_linkage, linked_vp, methods, and keys. It is concise and specific. Linked Issues check Issue #124requires changed errors in domain_linkage, linked_vp, methods, and keys to retain causes through source(), preserve reason context, document source behavior, and add tests. The PR adds opti…Out of Scope Changes check The changes stay within issue #124. Source-preserving changes cover the listed modules and related did:key handling uses the same error-cause rule. The added constructors, documentation, implementatio…Full details: Linked Issues check
-
Explanation
Issue
#124requires changed errors in domain_linkage, linked_vp, methods, and keys to retain causes through source(), preserve reason context, document source behavior, and add tests. The PR adds optional boxed sources or existing typed sources across the listed variants, keeps cause text in Display messages, updates docs/DESIGN.md, and adds downcast and no-source tests. The public API changes and constructors cover the stated breaking-change requirements.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#124. Source-preserving changes cover the listed modules and related did:key handling uses the same error-cause rule. The added constructors, documentation, implementation updates, and tests support the error API change. No unrelated behavior or feature is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 60.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 56 functions across 19 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! 🚀 New features to boost your workflow:
|
…imedes-echxvg-did-configuration-entries
…tries' into claude/inspiring-archimedes-echxvg-more-error-sources
|
@coderabbitai review Generated by Claude Code |
✅ Action performed
|
…imedes-echxvg-more-error-sources # Conflicts: # tests/domain_linkage.rs
Closes #124.
#118 made the
vc,sd_jwt_vcandjoseerrors keep their cause assource(), anddocs/DESIGN.mdstates the rule. These modules still formatted a wrapped error into aString, sosource()wasNoneand the cause survived only as text. Each one now keeps it.What changes
DomainLinkageErrorInvalidOrigin(String)→{ origin, reason, source }url::ParseError(not a URL) or the URI error (not RFC 3986); none for "no host" and "not a domain origin"DomainLinkageErrorInvalidService+sourceInvalidOrigin, or the URI or URL errorLinkedVpErrorInvalidService+sourceLinkedVpErrorInvalidBody+sourceUtf8Errorof a binary body, or theJwsErrorProduceErrorUnsupportedKey+sourcedid:jwk: theDidErrororResolutionError;did:key: theDidKeyError;did:web: theDidErrorof a badkidProduceErrorInvalidParams(String)→{ reason, source }did:web: theWebErrorof a URL that isn't adid:weblocationKeyStoreErrorInvalidKey+sourceJwkErrorof a public key that doesn't convert; software: thep256/k256error of an out-of-range scalar, or theJwkErrorof a missing thumbprintKeyStoreErrorInvalidKeySet+sourceserde_json::Errorof a file that isn't JSON, or the key error of one of its keysBox<dyn Error + Send + Sync>. Most variants mix several cause types, and theurl,p256andk256errors would otherwise become part of this crate's semver surface. That follows feat!: keep error causes assource()#118's rule.did:key: isn't in the issue's table, but it formatted itsDidKeyErrorthe same way, so I changed it too.cause()helper ends every message with its cause's, as feat!: keep error causes assource()#118 does, so no message loses information. A few now name the reason before the cause, for exampleinvalid origin "": not a URL: empty hostandkey 0 cannot be used: it has no did:key: ….source: None: "no host", "serviceEndpointis a map, not URLs", "useis notsig", and so on.InvalidOrigin(_); they now matchInvalidOrigin { .. }.For implementors outside the crate
DidProducerandKeyStoreare extension points. UniCore's own key store and DID methods buildProduceError::UnsupportedKey/InvalidParamsandKeyStoreError::InvalidKeythemselves, so adding a field breaks them. Three constructors cover the case without a cause:ProduceError::unsupported_key(index, reason);ProduceError::invalid_params(reason);KeyStoreError::invalid_key(key, reason).The crate's own sites without a cause use them. With a cause, implementors write the struct variant.
tests/resolver.rs's custom producer shows both, andDESIGN.mdnotes the convention.Smaller changes
LinkedVpError::InvalidBody:reasonis now&'static str. With aUrl, aStringand the source, the variant pushedLinkedVpErrorover clippy's large-error limit. Both of its reasons are fixed text anyway.Unusablestruct (reason + source) replaces theStringthe privateLocalKey::from_jwkandEntry::from_jwkreturned.for_keyandfor_key_setturn it intoInvalidKeyorInvalidKeySet.Breaking changes
The title is
feat!, so theSemVerjob skips this PR.cargo semver-checkslocally reports exactly these:DomainLinkageError::InvalidOriginandProduceError::InvalidParamswent from tuple to struct variants.sourceonDomainLinkageError::InvalidService,LinkedVpError::InvalidServiceandInvalidBody,ProduceError::UnsupportedKey, andKeyStoreError::InvalidKeyandInvalidKeySet.LinkedVpError::InvalidBody::reasonis now&'static str.None of these types derived
CloneorEqbefore, so no derives are lost.Tests
For each changed variant, a test checks
source()by downcasting, and that a variant built without a cause has none:domain_linkage:normalize_origin("mailto:…")has no source, with the exact new message;normalize_origin("")has aurl::ParseError;normalize_origin("`example.com")has aDidError(URI);servicewith that origin has anInvalidOrigin.linked_vp:DidError;Utf8Error;JwsError.Methods:
did:keywith an over-long key has aDidKeyError, with the message's new reason;did:jwkwith a key JCS can't serialize has aDidError;did:webwith anhttpURL has aWebError, and a badkidaDidError.resolver: a custom producer returns bothInvalidParamsforms; the constructors' messages, and that they have no source.keys:dis zero has ap256::ecdsa::Error;use: enchas no source;invalid_key's message;serde_json::Error.Two tests that matched on
reasontext now match onDisplay, which includes the cause.Checks
All with rustc/clippy 1.99.1, on the branch with
main(#125, #126, #127, #133) merged in:--all-features --all-targets -D warnings;cargo hack clippy --each-feature -D warnings;-D warnings;cargo test --all-features;--no-default-features --features did-key,softwarebuild;cargo publish --dry-run;cargo semver-checksas above..cargo/mutants.tomlpins no lines in the files this touches.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation