Open
Conversation
Adds two complementary aids for developing Presentation Definitions against a wallet's credentials (closes #4218): - Debug-level logs in vcr/pe.matchConstraints describing, per (input descriptor, credential), which credentials were rejected and the human-readable reason (which field/path/filter rejected, what value was found). Production hot path is untouched: gated on logrus.IsLevelEnabled(DebugLevel) and only adds work when on. - POST /internal/vcr/v2/holder/{subjectID}/match-explain — runs a given Presentation Definition against the holder's wallet and returns a structured MatchReport: per input descriptor the list of considered credentials with matched/reason, plus a satisfied/reason summary per submission requirement. Marked EXPERIMENTAL in the OpenAPI spec; may change or be removed without notice. Also fixes the existing log-level idiom in pe: log.Logger().Level on a logrus.Entry returns the entry's per-call level (zero by default) rather than the threshold, so debug guards built on it never trip. The new code uses logrus.IsLevelEnabled, matching the already-correct usage in vcr/api/vcr/v2/registry.go. Assisted by AI
reinkrul
requested review from
gerardsn,
stevenvegt and
woutslakhorst
as code owners
reinkrul
changed the title
Removes the experimental POST /internal/vcr/v2/holder/{subjectID}/match-explain
endpoint, the MatchReport API DTOs and the pe.PresentationDefinition.Explain
method. The debug-level logging in vcr/pe.matchConstraints is enough to
expose which credentials were considered and why each was rejected;
keeping the endpoint adds public API surface we'd need to support.
The reason-building helpers (explainConstraintMismatch,
explainFieldMismatch, credentialID, credentialToMap, fieldLabel) stay,
since the debug logging uses them.
Assisted by AI
|
Coverage Impact ⬇️ Merging this pull request will decrease total coverage on Modified Files with Diff Coverage (2)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
reinkrul
changed the title
Iterates on the debug logging added in the previous commit: - Drop the IsLevelEnabled gate. The trace is built unconditionally; logrus drops the entry when debug is off. The premature gate was not worth the branching. - Render the trace as a single human-friendly multi-line message via matchTrace.String() instead of attaching a struct as a logrus field (which renders awkwardly under both text and JSON formatters). - Format rejections now report what the credential offered alongside what each spec accepts, including cryptographic algorithm and proof type (e.g. "credential format=jwt_vc alg=ES384; presentation definition requires jwt_vc alg in [PS256]"). - Cover submission requirements: each requirement is appended to the trace with rule, from/from_nested, min/max/count, available count, and a satisfied/reason summary. errors.Join newlines in the reason are collapsed to ' — ' so the output stays cleanly indented. - Single log point in MatchWithSelector. matchBasic and matchSubmissionRequirements both return the trace alongside the result (including error paths), so the trace is logged regardless of which exit was taken; no defer needed. - Move all trace types (matchTrace, inputDescriptorTrace, rejectionTrace, submissionRequirementTrace) and their helpers (rejection-reason building, format describer, field-mismatch explainer) into vcr/pe/trace.go. presentation_definition.go is back to just the matching algorithm. Test file renamed explain_test.go -> trace_test.go. Assisted by AI
- Gate trace building on logrus.IsLevelEnabled(DebugLevel). The rejection-reason building (re-marshalling credentials, walking constraints) is too expensive to run unconditionally on Match's hot path, where it's called by discovery flows. - Don't abort the trace when a SubmissionRequirement returns an error: when debug is on, keep evaluating later requirements so the developer sees every requirement's outcome rather than just the first failure. Production fast-fail behaviour is preserved when debug is off. - Rename submissionRequirementTrace.AvailableInGroup -> Available so the JSON tag (\"available\") and the human log key (\"available=\") line up. - Update matchTrace.String() godoc to document the available=, from_nested= and (no id) tokens that were already emitted. - Pin the JWT alg in Test_formatRejectionReason so a fixture change fails the test loudly instead of silently. - Add TestMatchConstraints_DebugLogging_SubmissionRequirements_ContinuesAfterError to lock in the new \"keep going past SR errors\" behaviour. - Add TestMatchConstraints_NoTrace_WhenDebugDisabled to confirm no trace log line is emitted at info level. Assisted by AI
- Group the four trace-related test functions under one parent TestMatchConstraints_Tracing with subtests, sharing the logger setup, log-message extraction and SR fixture. - matchConstraints now returns its partial trace on its own internal errors (matchCredential / selector failures), matching what matchBasic and matchSubmissionRequirements already do. The trace built up to the point of failure is more useful for debugging than nothing. MatchWithSelector already nil-checks before logging. - Drop the unreachable fallthrough in describeFormatMismatch. The switch covers ldp_vc and jwt_vc — the only formats matchFormat inspects — so any other value would have already been rejected by the !formatAccepted branch. Returning "" rather than a generic message that can never fire. Assisted by AI
The trace structs were going to be serialized via the experimental match-explain endpoint that has since been dropped. They are now unexported, only consumed by matchTrace.String(), and never JSON-marshalled. The tags were dead weight. Assisted by AI
Replace the explicit nil-trace branching scattered through the
matching algorithm with a matchSink interface and two implementations:
- noopSink discards every event; used in production.
- traceSink accumulates a matchTrace and emits it on emit().
newSink picks one based on the current log level. The matching code
calls the sink unconditionally, so the algorithm itself is free of
debug-vs-production conditionals.
The submission-requirement loop also rewrites from the
"track-first-error-and-continue" pattern to two explicit passes:
evaluate everything (telling the sink), then aggregate with
first-error-short-circuit. The intent is now in the structure of
the code rather than encoded in sentinel tracking.
Net effect: the matching algorithm stays branch-free wrt tracing,
and the SR loop reads as intent ("report all; fail on first").
Assisted by AI
The four matchSink methods now take *InputDescriptor, *vc.VerifiableCredential and *SubmissionRequirement instead of copies, so the no-op sink (used in production) doesn't pay for the struct copy on every call. Trace-only helpers that the sink calls into (credentialID, credentialToMap, rejectionReason, formatRejectionReason, describeCredentialFormat, describeFormatMismatch, buildSubmissionRequirementTrace) are updated to match. The existing matching primitives (matchFormat, matchCredential, matchConstraint, matchField, matchFilter) are untouched. Assisted by AI
3 new issues
|
Two readability fixes for the debug trace: - Show overall outcome on the header: "PE: match evaluated (satisfied)" / "(not satisfied)". MatchWithSelector hands the result to sink.emit(). - Collapse $.type rejections. When a credential is rejected by an input descriptor's $.type filter, don't list it per-credential — for a wallet of N credentials and a PD with M descriptors that's N*M lines of unsurprising noise. Instead, if a descriptor ends up with matched=0 and no other rejections were observed, emit a single "no credentials matched the type X" line. Other field rejections (id pattern, missing required value, etc.) still surface per-credential — those are usually the bug being chased. Assisted by AI
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.
Closes #4218.
Summary
When debug-level logging is enabled, every (input descriptor, credential) pair the Presentation Exchange matcher considers is logged with a human-readable reason for rejection — which field/path/filter rejected, what value was found, what the filter expected. Plus a per-input-descriptor summary line showing how many credentials were considered, how many matched, and which one was selected.
Production hot path is untouched: gated on `logrus.IsLevelEnabled(DebugLevel)` and only does the extra reason-building work when debug is on.
Example debug output for a non-matching VC:
```
DEBU[…] PE: credential rejected credential=urn:test:non-matching input_descriptor=as_jsonld module=VCR reason="no value found at any of paths [$.credentialSubject.organization.city]"
DEBU[…] PE: input descriptor evaluated considered=2 input_descriptor=as_jsonld matched=1 module=VCR selected="did:nuts:CuE...#ec8af8cf-…"
```
A debug-only endpoint exposing the same info as a structured `MatchReport` was proposed and prototyped, but dropped from this PR — too much public-API surface for what is essentially a development aid. The pe-internal helpers that build the reasons stay, and we can revisit the endpoint later if logs prove insufficient.
Drive-by fix
The existing `log.Logger().Level >= logrus.DebugLevel` idiom (e.g. `auth/services/irma/signer.go`, `network/dag/notifier.go`) is broken: `Level` on a `*logrus.Entry` is the per-call level (zero by default) rather than the logger's threshold, so the guard never trips. New code uses `logrus.IsLevelEnabled(...)`, matching the already-correct usage in `vcr/api/vcr/v2/registry.go`. Existing call sites are untouched — narrow scope.
Out of scope
Test plan
Assisted by AI