Repository navigation
Conversation
Add an explicit output content type to the credential option descriptor and the issue request, a distinct IssueCredentialV2 RPC, and a distinct typed task arm for the task-backed transport. The type is a distinct method and a distinct task arm rather than a new field on the existing ones because protobuf unknown-field tolerance lets an implementation that predates this contract ignore the field and mint legacy bytes under a typed request. A runtime without the method, or without the task arm, refuses before its Issue implementation runs and before any provider call, so protocol identity supplies the fence instead of a deployment check that a mutable alias can race. Field numbers verified free and unreserved at this revision: - CredentialIssueOptionDescriptor.output_content_type = 11 (1-10 in use) - IssueCredentialRequest.output_content_type = 6 (1-5 in use) - Task.IssueTypedCredentialTask = 124 (123 is issue_credential; 119 and "create_sync_diff" are reserved) Generated bindings follow in a separate commit.
Generated from the preceding source commit with `make protogen`. Adds IssueCredentialRequest.output_content_type, the IssueCredentialV2 RPC, the descriptor's output_content_type, and the IssueTypedCredentialTask arm. Generated code is committed separately from the hand-written proto source so a rebase of either stays clean.
…t type The SDK's existing vocabulary calls secret_resource_type_id an option's "output type". The new field names the encoding of the credential's value bytes, which is a separate axis. Say so where a reader meets both fields, so the two cannot be conflated when the contract is implemented.
Comment-only regeneration from the preceding source commit.
Serve IssueCredentialV2 through the shared implementation with a typed flag, so the legacy method keeps its exact behaviour and the typed method cannot be reached by accident. The executing descriptor is the authority on what a connector can produce, so a typed request is checked against it before Issue runs: - a matching declaration reaches Issue and is carried on CredentialIssueInput - a mismatched declaration is FailedPrecondition (contract drift) - an option that declares nothing is FailedPrecondition (unavailable, not downgraded) - an empty type on the typed path, or a nonempty type on the legacy path, is InvalidArgument Every refusal happens before the provider is touched, which the tests assert by having the fake issuer record its input only inside Issue. There is no fall back to the legacy method on any error. Also add the distinct typed task arm and handler, the IssueTypedCredentialType task type (appended to preserve released ordinals), and the method on the empty and noop connectors so the generated server's Unimplemented default stays reachable. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
General PR Review: feat: typed credential output contract (IssueCredentialV2)Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThis PR adds three new proto fields. Repo criteria applied, with risk triage:
Verdict: HIGH. The PR does include a permutation table: the builder tests cover all five match/mismatch/undeclared/malformed combinations and assert that Coverage: I reviewed every file in the PR. I did not run builds or tests; the compile break below comes from reading the code. Security IssuesNone found. Correctness Issues
Suggestions
Prompt for AI agents |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // answers Unimplemented before its Issue implementation runs and before any | ||
| // provider call, so the method's existence is the fence. Callers must never | ||
| // fall back to IssueCredential on Unimplemented or any other error. | ||
| rpc IssueCredentialV2(IssueCredentialRequest) returns (IssueCredentialResponse); |
There was a problem hiding this comment.
🟠 Bug: Adding this RPC adds IssueCredentialV2 to CredentialManagerServiceServer, which has no mustEmbedUnimplemented… requirement. internal/connector/noop_provisioner.go doesn't implement it, so connectorV2.RegisterCredentialManagerServiceServer(s, noop) at internal/connector/connector.go:517 won't compile, and that breaks every binary that imports internal/connector. Add func (n *noopProvisioner) IssueCredentialV2(...) returning FailedPrecondition, "provisioning is not enabled". That also keeps the provisioning-disabled path refusing typed issuance the same way it refuses the legacy RPC. (Confidence: high)
| return h.helpers.FinishTask(ctx, nil, nil, errors.Join(errors.New("malformed issue typed credential task"), ErrTaskNonRetryable)) | ||
| } | ||
|
|
||
| resp, err := h.helpers.ConnectorClient().IssueCredentialV2(ctx, v2.IssueCredentialRequest_builder{ |
There was a problem hiding this comment.
🟡 Suggestion: Nothing tests this handler or the IssueTypedCredentialType dispatch in manager.go. Here are two regressions that would ship without a test catching them: the handler calls IssueCredential instead of IssueCredentialV2, or OutputContentType gets dropped from the built request. Either one silently mints legacy bytes under a typed task. Mirror issue_credential_test.go with a fake client that asserts the V2 method was called and the content type was forwarded, plus a case for the malformed task. (Confidence: high)
A failure after Issue returned means the provider object already exists, but three post-Issue paths returned an error with no handle for it: cardinality validation, output validation, and encryption. The caller could neither revoke the object nor tell whether retrying was safe. Carry the provider identity in the error and say plainly that issuance must not be retried. This does not add a discard point; it makes the existing one recoverable, and it preserves the ambiguity contract the task handler already relies on when it marks issuance errors non-retryable. The test mints a real object and then returns undeliverable output, and asserts the handle appears in the error.
General PR Review: feat: typed credential output contract (IssueCredentialV2)Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThis PR adds a typed issuance path:
The builder refuses mismatched, undeclared, and malformed requests before I scanned the full diff for security and correctness. I checked the generated Risk triage:
Verdict: HIGH (version-pair). The risk is in the multi-artifact class. The instrument that would cover it is a two-artifact harness: a new caller against an old server and an old task manager, asserting that the old side refuses before Security IssuesNone found. Correctness Issues
Suggestions
Coverage notesI reviewed the generated Prompt for AI agents |
…k routing The generated Unimplemented default only proves what a default does when the method is registered. An executor built before the contract never registers it, so the downgrade regression serves a real gRPC descriptor with the typed method filtered out and asserts the typed call is Unimplemented with the legacy Issue entrypoint never reached. The legacy route on the same old executor still works, and an executor that registers the method serves the typed call without touching the legacy one. On the task side, the typed arm resolves to its own task type, the typed handler dispatches the typed method, and the legacy handler cannot service a typed task at all: it reads a nil IssueCredential and refuses as non-retryable with no call. Also pin that an upgraded SDK leaves legacy issuance alone: an untyped request still reaches Issue with an empty contract and keeps the connector's own name and raw bytes on the encrypted result.
Superseded — see the current review report for commit
|
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
…rose A concatenated resource id in an error message is not a recovery contract: a caller would have to parse English to find the object it has to clean up. Attach the sanitized ResourceId as a structured gRPC status detail instead, alongside an ErrorInfo naming the reason and marking retry forbidden and cleanup required. No credential material and no profile values go into the details. The message no longer claims a live object exists. A failure after Issue means the outcome is unresolved and an object may exist; when no identity came back the status carries no detail, which callers must read as unresolved cleanup rather than proof that nothing was created. Prove the detail survives the path a real failure takes: the builder's status error is joined with the task's non-retryable marker, and the joined error still exposes the ResourceId through status.FromError. If joining had dropped it, C1 would silently lose the handle again.
Superseded — see the current review report for commit
|
| st, _ = st.WithDetails(&errdetails.ErrorInfo{ | ||
| Reason: "CREDENTIAL_ISSUED_BUT_UNDELIVERABLE", | ||
| Domain: "baton-sdk/credentialbuilder", | ||
| Metadata: map[string]string{ | ||
| "cause": cause.Error(), | ||
| "retry": "forbidden", | ||
| "cleanup": "required", | ||
| }, | ||
| }) | ||
| if output != nil && output.Secret != nil && output.Secret.GetId() != nil { | ||
| st, _ = st.WithDetails(output.Secret.GetId()) | ||
| } | ||
| return st.Err() |
There was a problem hiding this comment.
🟠 Bug: The WithDetails error is discarded, but on failure it returns (nil, err). A nil *Status has Code() == OK, so st.Err() returns nil. The case that triggers this: a proto3 string with invalid UTF-8, such as a provider error text in cause.Error() or a connector-supplied ResourceId. anypb.New then fails, and issueCredential returns (nil, nil) after a mint, so the failure is silently treated as success. Keep the base status when attaching fails, e.g. if withInfo, err := st.WithDetails(...); err == nil { st = withInfo }, and do the same for the ResourceId. (Confidence: high)
| // that nothing was created. | ||
| func credentialIssuedButUndeliverable(output *CredentialIssueOutput, cause error) error { | ||
| st := status.New(codes.Internal, | ||
| "credential was minted but cannot be delivered; the provider outcome is unresolved and issuance must not be retried") |
There was a problem hiding this comment.
🟡 Suggestion: The underlying cause used to be in the status message and now appears only in ErrorInfo.Metadata["cause"]. err.Error() prints only the fixed message, so task-failure logs and anything else that logs the error no longer show why delivery failed (e.g. an encryption or cardinality failure). Either log cause here with zap before returning, or append a sanitized cause to the message. (Confidence: medium, observability)
Superseded — see the current review report for commit
|
There was a problem hiding this comment.
Blocking issues found — see the full review report
The noop provisioner is a registered CredentialManagerService, so the new method has to exist on it. It answers FailedPrecondition like the legacy IssueCredential, because provisioning is not enabled either way.
Superseded — see the current review report for commit
|
General PR Review: feat: typed credential output contract (IssueCredentialV2)Blocking Issues: 1 | Suggestions: 3 | Threads Resolved: 0 Review SummaryThe PR adds a separate I applied the trusted criteria:
Security IssuesNone found. Correctness Issues
Suggestions
Resolved prior findings
Prompt for AI agentsReviewed commit: |
There was a problem hiding this comment.
Blocking issues found — see the full review report
![C1 Squire [DEV]](https://avatars.githubusercontent.com/in/2846581?s=40&u=9dfdc6298bb22a241aaf41a6558bc1563c5c6466&v=4){kind=avatar}
|
Squire (claude-opus-5-5): Closing as superseded. This PR uses request field numbers 5 and 6 for |
|
Squire (claude-opus-5-5): The replacement is #1218. |
Description
Adds an explicit typed output contract to credential issuance, so a caller can
require that the bytes a connector mints conform to a named
SecretPayloadV2content type — and so an implementation that predates the contract cannot
silently mint legacy bytes under a typed request.
The contract is carried by protocol identity, not by a deployment check:
CredentialManagerService.IssueCredentialV2is a distinct RPC. A runtimewithout it answers
Unimplementedbefore itsIssueimplementation runs andbefore any provider call. The generated default
(
UnimplementedCredentialManagerServiceServer.IssueCredentialV2) is thatfence.
Task.IssueTypedCredentialTask(arm 124) is a distinct task arm for thetask-backed transport, with its own handler and its own task type. An older
task handler does not recognise the arm at all.
Unimplemented, timeout, orany other error. A typed issuance that cannot be served is an error.
The builder checks the request against the executing descriptor before
Issueruns: a mismatch isFailedPrecondition(contract drift), an optionthat declares nothing is
FailedPrecondition(unavailable, not downgraded), anda malformed pairing — an empty type on the typed path or a nonempty type on the
legacy path — is
InvalidArgument. Every refusal happens before the provider istouched.
Field numbers, verified free and unreserved at this revision:
CredentialIssueOptionDescriptoroutput_content_typeIssueCredentialRequestoutput_content_typeTask.IssueTypedCredentialTaskissue_credential; 119 andcreate_sync_diffare reserved)output_content_typeis notsecret_resource_type_id. That field names theC1 resource type an option mints — the SDK's existing "output type" — and
selects which descriptor handles a request. This one names the encoding of the
credential's value bytes. The proto comments say so on both fields.
Empty is meaningful and is the legacy contract on both sides, so an old caller
talking to a new connector and a new caller talking to an old connector both
keep their existing meaning.
Testing
New tests cover: a matching declaration reaches
Issueand is carried onCredentialIssueInput; a mismatched declaration, an undeclared option, and amalformed pairing are each refused with the right code; and the legacy path is
unchanged. Each refusal asserts the fake issuer never saw the request, so "refused
before mint" is proven rather than asserted.
The task-type ordinal is pinned by the existing released-ordinal test
(
IssueTypedCredentialType= 33).Notes
can be rebased cleanly.
typed method, populate the advisory content type for reveal discovery) is a
separate PR and is not in this one.