Skip to content

feat(connectorbuilder): resolve credential issuers on shape + output type - #1109

Merged
highb merged 12 commits into
mainfrom
highb/credential-issuer-output-type
Aug 28, 2026
Merged

highb merged 12 commits into
mainfrom
highb/credential-issuer-output-type

Conversation

@c1-squire-dev

@c1-squire-dev c1-squire-dev Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

IGA-4107

What

A connector can advertise at most one credential issue option per credential shape (API_KEY / KEYPAIR / TOKEN / CLIENT_SECRET). Registration dedupes on that shape alone, so a provider that mints two credentials sharing a shape can only ever offer one of them.

Datadog is the live case: an organization API key and a service-account application key are both API-key-shaped. baton-datadog hit this and shipped only the service-account half.

The discriminator already exists and is already mandatory. Registration rejects a descriptor with an empty secret_resource_type_id, so every descriptor already declares the resource type its credential comes back as. Dedup and resolution simply never looked at it.

This identifies an issuance option by the pair the connector already declares: its shape and its output type — and names which of them is the default, so a caller presenting a choice has one selected.

The four edits

1 · Dedupe on (option, secret_resource_type_id) — validateCredentialIssueCapabilityDetails, pkg/connectorbuilder/connectorbuilder.go. Two descriptors sharing a shape with distinct output types both register; the same output type twice is still a duplicate.

2 · Resolve on the same pair, and require it — resolveCredentialIssueDescriptor in pkg/connectorbuilder/credential_issue_validation.go replaces the first-shape-match loop. Both halves of the selection are required, so the pair names at most one advertised descriptor and resolution is a lookup, not a search with fallbacks.

An earlier revision of this PR treated an empty output type as "the caller predates this field" and resolved the sole descriptor for the shape. That branch serves nobody: Task.IssueCredentialTask has no producer anywhere, and c1's only issuance call site already hard-fails on an empty value before it builds a request. Meanwhile the fallback's ambiguity error was reachable only once a connector used the new capability — i.e. it existed to soften exactly the case this PR exists to enable. Requiring the field now is the reversible direction; optional → required later would be a breaking tightening.

The requirement is enforced in Go, not in proto. This repo has no protovalidate dependency and the connector server's interceptor chain is recovery-only, so a min_bytes: 1 rule would read as enforcement and do nothing — pkg/dotc1z/grants.go:448 already says so about principal_id. The same reasoning applies to a length bound, so CredentialIssueOptions.secret_resource_type_id carries no validate.rules at all: presence and the 1024-byte cap are both checked in resolveCredentialIssueDescriptor, and the proto comment names that as the enforcement point. Keeping a decorative max_bytes beside a deliberately-omitted min_bytes would have been the inconsistency.

3 · Carry the output type inside CredentialIssueOptions — string secret_resource_type_id = 1, beside the oneof (arms start at 100, so field 1 was free for exactly this).

CredentialIssueOptions is already a selector: its oneof arm is what determines the shape, which is why credentialIssueOptionKind has to switch on WhichOptions(). Putting the output type on IssueCredentialRequest as a sibling of credential_options split one selection across two fields at two levels, which could be read from different places and forwarded independently. On the options message the selection is a single value that travels wherever the options do.

That is a strict reduction, not a relocation. It deleted the second wire hop this PR previously needed:

  • Task.IssueCredentialTask.secret_resource_type_id — gone. The task already carries credential_options = 2, so the discriminator rides inside it for free.
  • The forwarding line in pkg/tasks/c1api/issue_credential.go — gone; that file is now byte-identical to main.
  • Its two tests ("forwards the task's secret resource type", "a task with no secret resource type sends none") — gone, along with TestValidateCredentialIssueInputWithoutOutputTypeIsUnchanged, whose premise was false.
  • CredentialIssueInput.SecretResourceTypeID — not added. A flat copy beside CredentialOptions would reintroduce the same split in Go, where the two could disagree. Connectors read input.CredentialOptions.GetSecretResourceTypeId(). pkg/connectorbuilder/credentials.go is also back to main.

Net: the hand-written diff against main is now three files plus the new test file.

4 · Name the default descriptor within a shape — bool preferred = 10 on CredentialIssueOptionDescriptor. preferred_option alone stopped naming one descriptor the moment two could share a shape; the flag on the descriptor completes the selection, and registration enforces that it always resolves. Detail in What preferred_option means now below.

The candidates that turned out to already be done

The brief flagged three things worth checking rather than assuming. All three already hold, so none of them is a separate edit:

Candidate Status
Issued secret's resource type validated against the declared secret_resource_type_id Already enforced — validateCredentialIssueOutput, credentials.go. Now stronger: the descriptor it checks against is the one the caller selected, so "you got what you asked for" is enforced per kind.
ResourceDeleterV2 registered for the declared output type ("revocability by construction") Already enforced — the descriptor loop in GetCapabilities. It iterates every descriptor, so it already covers the new multi-descriptor case. Test added to pin that.
connector_min_expiry carried Already present — IssuanceExpiryCapability has both min and max, and validateCredentialIssueInput enforces both. The design's open question is about the C1-side model not capturing min, not the SDK.

Backward compatibility

Registration is fully compatible. A one-descriptor-per-shape declaration has a unique (option, secret_resource_type_id) pair by construction, so widening the key can only accept declarations that used to be rejected. Nothing that registered before stops registering, and no connector needs a change. buf breaking --against main passes: CredentialIssueOptions.secret_resource_type_id is a new optional field, and the two fields the earlier revision added never shipped.

Callers must now send the output type. This is a deliberate break of the earlier revision's fallback, not of anything released, and it is safe because there is no production caller to break:

  • A connector is the server; it never constructs an IssueCredentialRequest. Every existing connector keeps working untouched.
  • Task.IssueCredentialTask has no producer in c1 at any ref — taskToAPI has no Task_IssueCredential case — so the queued-task path is not live.
  • c1's one intent-bearing call site (connector_actions_v2.go) already resolves the output type and already rejects an empty one. It just does not send it yet. That is the caller-side half, and it is a separate c1 PR.

What preferred_option means now, and the flag that completes it

CredentialDetailsCredentialIssue.preferred_option is a bare CapabilityDetailCredentialOption. Once two descriptors can share a shape, a preferred option alone can no longer name which descriptor is preferred. preferred_option = API_KEY against a connector advertising both an org API key and a service-account application key does not say which one.

A UI is being built against this, and it needs a default selected in the picker. So selection is now explicitly two-level, and both levels ship here:

  • preferred_option picks the shape. Unchanged meaning, unchanged validation: it must match the shape of at least one declared descriptor.
  • CredentialIssueOptionDescriptor.preferred (bool, field 10, new) picks the descriptor within that shape.

Together they always resolve to exactly one default. The flag lives on the thing preferred, so it cannot dangle the way a companion preferred_secret_resource_type_id on the parent could — that alternative is a two-part pointer into a repeated field that must jointly resolve to one element, replacing a validation that can dangle one way with one that can dangle three.

Registration enforces the guarantee rather than documenting it (validateCredentialIssuePreference):

Declaration Result
One descriptor for a shape, preferred unset accepted — the sole descriptor is the default
One descriptor for a shape, preferred set accepted
Several descriptors share a shape, exactly one preferred accepted
Several descriptors share a shape, none preferred rejected
Several descriptors share a shape, two or more preferred rejected

The two rejections cost no existing connector anything: a declaration with two descriptors sharing a shape is one that main rejects outright, so every declaration that registers today is in the first two rows. Requiring the flag exactly where ambiguity is possible is what lets a consumer assume a default always exists — a default the SDK picked out of declaration order would not be stable across a connector's own re-declaration.

The rule a picker implements. preferred is required only where a shape has several descriptors, so a single-descriptor shape is the default with the flag left unset — not as a legacy state, but as the permanent steady state for most connectors, since nothing will ever oblige them to set it:

Take the descriptors carrying preferred_option. One of them has preferred set, except where that option has a single descriptor, which is the default with the flag unset.

Scanning the whole options list for preferred is the wrong implementation: it can land on a descriptor outside the preferred shape. The rule is stated on preferred_option in the proto, which is where a consumer starts.

The alternative — require exactly one preferred descriptor per shape unconditionally — gives a consumer a uniform rule with no fallback, and was rejected: it rejects every connector registering today, since none of them can set a field that does not exist yet. That is a fleet-wide registration break, an order of magnitude wider than the caller-side ordering constraint below.

Preference is scoped per shape, not per declaration. A connector advertising two API keys and two tokens marks one of each, so a picker that lets the user switch shape still has a default in the new shape.

preferred selects nothing at issue time. It is advisory for a caller presenting a choice; CredentialIssueOptions.secret_resource_type_id is still required on every request, and resolution is still an exact lookup with no fallback. Making the flag a resolution fallback would reintroduce exactly the implicit selection edit 2 removed. There is a test pinning that an empty output type still fails even when a preferred descriptor exists.

Not deprecating preferred_option. It shipped in v0.25.1 and is required at registration; it now has a well-defined narrower job (pick the shape) that composes with the new flag. Removing it is a wider decision on released public API and is not carried here.

No Go helper resolving the pair. The rule is in the proto comment and enforced at registration, but there is no exported PreferredDescriptor(details) in this PR: the consumer is c1's capability-to-offering mapping, so an SDK helper would ship with no in-repo caller. Unlike the field placement, a Go function is additive whenever it is added — the same asymmetry that put the proto field in now keeps the helper out.

Consumers — checked, not changed

baton-datadog (santhosh.kumar/credential-issuance) — verified against a local replace (not committed; the working tree is back to clean). Built the real connector with a second descriptor for api-key alongside service-account-application-key, then ran the SDK's registration path via GetMetadata → GetCapabilities:

  • against main: duplicate credential issue option CAPABILITY_DETAIL_CREDENTIAL_OPTION_API_KEY
  • against this branch: both options advertised — [service-account-application-key api-key]

Both output types already have registered deleters (apiTokenBuilder for api-key, applicationKeyBuilder for service-account-application-key), so the revocability invariant holds for both without further work. The connector also compiles unmodified against this branch. Restoring its org-API-key half is a separate PR.

c1 — consumes this through its vendored copy (pinned v0.25.1, identical to main here). Nothing in c1 was modified by this PR, and nothing in c1 breaks by merging it, because the issuance dispatch path is not live.

The caller-side fix is a separate c1 PR, and it is a real defect rather than new plumbing: resolveCredentialIssueResourceTypeFromCapabilities (pkg/temporal/activity/.../connector_actions_v2.go) already resolves the expected output type and hard-fails on empty, the request it then builds omits it and hardcodes the ApiKey arm, and the response is validated against the value that was never sent. With two same-shape descriptors that mints the wrong credential at the provider and then rejects it — a real key created, then discarded. That PR sets the field on CredentialIssueOptions and builds the arm from the offering's credential_option.

Rollout ordering

secret_resource_type_id is required on every issuance request once this merges, not only against connectors that declare two same-shape descriptors. So the constraint is a single one, and it is about vendoring order, not about connector declarations:

Do not vendor this SDK version into c1 until the caller-side PR that populates the field lands. That PR is ductone/c1#24250, held in draft for exactly this reason.

Cut this as a minor bump, v0.26.0, not a patch. It changes the default behaviour of an RPC shipped in v0.25.1 — a request that validated before now fails InvalidArgument. A downstream ~v0.25 constraint must not pick this up silently. pkg/sdk/version.go is generated from the tag, so this is an ask on the release tag rather than a file in this diff.

Stating this here and in the release note rather than in the proto: the proto comment already says the field is required, which is the whole of what a connector author needs, and a release-coordination fact between two specific pull requests goes stale in source the moment they land.

The narrower phrasing — "declare a second same-shape descriptor only once callers populate the field" — is not the right note, because it understates the requirement. A single-descriptor connector is affected too.

Verification

  • go test ./... (-tags=baton_lambda_support): all packages pass.
  • golangci-lint run: 0 issues. go vet: clean.
  • buf lint, buf format: clean. buf breaking --against main: passes.
  • Negative control: reverting the dedup key to shape-only fails the new registration test; the baton-datadog check above fails against main.
  • Preference tests cover every row of the table above, plus preference scoped per shape rather than per declaration, plus a rejection that names the offending shape when another shape is well-formed, plus preferred selecting nothing at issue time.
  • Tests cover: same-shape/distinct-output registration, same-shape/same-output still rejected, the pair selecting one descriptor among same-shape descriptors, a missing output type rejected before the provider is mutated, an undeclared output type rejected, an undeclared shape rejected, options with no arm set rejected, and a deleter required for every declared output type.
  • Tests also cover the case same-shape descriptors that differ only in their output type cannot: two API-key descriptors advertising scopes: ["read"] and scopes: ["write"], asserting each scope is accepted by the descriptor advertising it and rejected by the other, both through validateCredentialIssueInput and end-to-end through IssueCredential. A shape-only regression passes the selection tests and fails these.
  • The 1024-byte cap on secret_resource_type_id is checked on both sides in Go — at registration in validateCredentialIssueCapabilityDetails and per-request in resolveCredentialIssueDescriptor — so an over-long output type fails fast at registration rather than on every issuance.
  • The two resolution failures are now distinguished: an unadvertised shape reports only the shape, and an unadvertised output type names the ones that shape does produce.

Local toolchain note: go1.27 cannot build the vendored cockroachdb/swiss, which build-tags [go1.20, go1.27). Pre-existing on main and unrelated to this change; everything above ran under GOTOOLCHAIN=go1.26.0.

CI: 12/12 green as of 85ca3770; re-running against 1ff7ce2d.

highb and others added 2 commits August 27, 2026 00:33
…t type

A connector could advertise at most one credential issue option per
credential shape, so a provider minting two API-key-shaped credentials --
Datadog's organization API key and its service-account application key --
could only ever offer one of them.

Identify an issuance option by the pair the connector already declares:
its shape and the resource type the minted credential comes back as.

- Registration dedupes on (option, secret_resource_type_id) rather than
  option alone, so two same-shape options with distinct output types both
  register.
- IssueCredentialRequest and the issue-credential task carry
  secret_resource_type_id, and CredentialIssueInput surfaces it so a
  connector can branch on which kind was requested.
- Issue-time resolution matches on the same pair. An absent output type
  resolves the sole descriptor for the shape, exactly as before, and is
  rejected as ambiguous only where a connector advertises more than one
  descriptor for that shape -- a declaration registration used to reject
  outright. Every existing connector and caller is unaffected.

preferred_option stays a bare shape. It now expresses no preference among
descriptors sharing that shape; naming one needs a companion field, left
out until a caller consumes it.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment thread pkg/connectorbuilder/credentials.go Outdated
CredentialOptions: request.GetCredentialOptions(),
ExpiresAt: request.GetExpiresAt(),
RequestID: request.GetRequestId(),
SecretResourceTypeID: request.GetSecretResourceTypeId(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: input.SecretResourceTypeID stays the caller's raw value, so a connector that branches on it still receives "" on the legacy single-descriptor path and must re-derive which descriptor the SDK actually selected. Since validateCredentialIssueInput already resolved a unique descriptor, consider normalizing after validation (input.SecretResourceTypeID = descriptor.GetSecretResourceTypeId()) so Issue always sees the resolved type — a connector that guesses wrong only finds out via validateCredentialIssueOutput, i.e. after the provider was already mutated and with no automatic retry, leaving an orphaned credential. The field is new in this PR, so nothing depends on it being empty today.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squire (claude-opus-5): This one is moot at the current head rather than fixed — flagging that rather than inventing a change.

CredentialIssueInput.SecretResourceTypeID was never added. The revision that moved the discriminator onto CredentialIssueOptions (0873c7c) dropped it, for the same reason the comment identifies: a flat copy beside CredentialOptions is a second place the selection can live and disagree with the first. pkg/connectorbuilder/credentials.go is byte-identical to main.

The normalisation the comment asks for is structural now: connectors read input.CredentialOptions.GetSecretResourceTypeId(), which is the value resolveCredentialIssueDescriptor matched the descriptor on, so there is no raw-versus-resolved pair to reconcile. Leaving unresolved for a human to close.

Comment on lines 128 to +132
// Resource type returned by IssueCredential. It must be registered with a
// ResourceDeleterV2 so every issued credential has a provider revoke path,
// including virtual credentials that cannot be listed later.
// including virtual credentials that cannot be listed later. Together with
// option it identifies this descriptor, so two credential kinds sharing one
// shape must return distinct resource types.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: worth stating the rollout ordering here. Once a connector declares a second descriptor for a shape, every request that omits secret_resource_type_id for that shape starts failing as ambiguous — including the one that works today. Registration accepts the new declaration silently, so the break only surfaces at issue time, and the PR notes C1 does not populate the field yet. A sentence saying "declare a second same-shape descriptor only once callers populate IssueCredentialRequest.secret_resource_type_id" would put the constraint where connector authors will read it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squire (claude-opus-5): Declined for the proto comment; the constraint is now recorded in the PR body under "Rollout ordering".

Two reasons. The narrower phrasing this asks for understates the requirement at the current head: secret_resource_type_id is required on every issuance request, not only against a connector that declares a second same-shape descriptor, so a single-descriptor connector is affected too. And the actual constraint is about vendoring order between two specific pull requests — do not vendor this SDK version into c1 before the caller-side PR populates the field — which goes stale in source the moment both land.

The proto comment already says the field is required, which is the whole of what a connector author reading it needs. Leaving unresolved for a human to disagree with.

google.protobuf.Timestamp expires_at = 4;
// Forwarded verbatim to IssueCredentialRequest.secret_resource_type_id.
// Empty keeps the pre-existing shape-only selection.
string secret_resource_type_id = 5;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: this field carries no validate.rules, while the IssueCredentialRequest.secret_resource_type_id it forwards into is bounded at max_bytes: 1024. Neither bound is actually enforced on the serving path — IssueCredentialRequest.Validate() is never called — so unlike request_id, whose proto rule is mirrored by hand in validateCredentialIssueInput, this one is decorative. Low impact (an oversized value just fails to match a descriptor), but mirroring the 1024-byte cap here and/or in validateCredentialIssueInput would keep the two hops consistent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squire (claude-opus-5): Moot at the current head, not fixed by this pass.

The field this targeted — Task.IssueCredentialTask.secret_resource_type_id = 5 — no longer exists; 0873c7c removed it along with its "Forwarded verbatim" comment when the discriminator moved onto CredentialIssueOptions, which the task already carries as credential_options = 2. proto/c1/connectorapi/baton/v1/baton.proto is byte-identical to main.

The consistency point survives the move and was taken on the field that replaced it — see the resource.proto:234 thread. Leaving unresolved for a human to close.

@github-actions

github-actions Bot commented Aug 27, 2026 •

Copy link
Copy Markdown
Contributor

General PR Review: feat(connectorbuilder): resolve credential issuers on shape + output type

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 729390986af8.
Review mode: incremental since 60f3e133
View review run

Review Summary

The new commits change only the preferred_option doc comment in proto/c1/connector/v2/connector.proto plus its regenerated pb/ mirrors, and that comment now states the sole-descriptor fallback the previous review flagged — a consumer resolving the default from the descriptors carrying preferred_option gets the right answer both when several descriptors share the shape and when only one does, so that finding is addressed. The full PR diff, including the two generated files the incremental artifact filtered out (pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go), was scanned for security and correctness: those generated changes are a faithful regeneration — the rawDesc length deltas match the encoded field bytes exactly, +30 for preferred = 10 and +55 for secret_resource_type_id = 1 — and no new security or correctness issues were found.

Risk triage: durable and cross-consumer (proto wire types read by c1 and by future SDK versions), but both proto changes are new optional fields on released messages occupying previously-unused numbers, and widening the registration key from option to (option, secret_resource_type_id) can only accept declarations that main rejected, so nothing that registers today stops registering and the new preferred requirements are unreachable for every currently-valid declaration. The one real break — CredentialIssueOptions.secret_resource_type_id now required on every issuance request, with the v0.26.0 minor-bump and ductone/c1#24250 vendoring-order constraints that follow from it — is already captured in the open threads on credential_issue_validation.go:46 and resource.proto:236 and is not re-raised here.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.


Reviewed head 1ff7ce2d6fda32de4135ddc7ad34e338e9693285 against base 729390986af84d404fdd6502681fe22fb85840b0. The machine-readable review-state marker could not be written on this run — the review sandbox blocked both file writes and the inline JSON — so the next run will not find prior state and should be treated as a full review rather than an incremental one.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

highb and others added 2 commits August 27, 2026 16:20
…lIssueOptions

The discriminator sat on IssueCredentialRequest as a sibling of
credential_options, splitting the descriptor selection across two fields:
the oneof arm gave the shape, the request field gave the kind within it.
Move it onto CredentialIssueOptions beside the oneof so the selection is
one value that travels wherever the options do.

Task.IssueCredentialTask.secret_resource_type_id and its forwarding in the
c1api handler go away with it -- the task already carries credential_options,
so the discriminator rides along for free.

Make it required and drop the empty-value resolution fallback. No caller
predates the field, and the only real caller already rejects an empty value,
so resolution collapses to a lookup on (option, secret_resource_type_id).
The check is in Go, not a proto rule: this repo has no protovalidate
dependency and the connector interceptor chain is recovery-only, so a
min_bytes rule would read as enforcement and do nothing.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment thread proto/c1/connector/v2/resource.proto Outdated
Comment on lines +36 to +39
secretResourceTypeID := options.GetSecretResourceTypeId()
if secretResourceTypeID == "" {
return nil, fmt.Errorf("credential_options.secret_resource_type_id is required")
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion (rollout ordering): this makes secret_resource_type_id unconditionally required on a request contract that shipped in v0.25.1, so the requirement is global, not scoped to connectors that declare a second same-shape descriptor. Any caller that omits it fails with InvalidArgument — including single-descriptor connectors that work fine on main today. I can't verify the "no live producer" claim from this repo (the SDK's own producer at pkg/tasks/c1api/issue_credential.go:43 forwards credential_options wholesale, so it's clean), and the failure is loud and lands before issuer.Issue mutates the provider, which is the right failure shape. The residual is ordering: this SDK version must not be vendored into c1 before the caller-side PR that populates the field. Worth stating that constraint in the PR/release note so the two land in order.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Squire (claude-opus-5): Agreed, and the constraint is now stated in the PR body under "Rollout ordering" rather than in a comment: do not vendor this SDK version into c1 until the caller-side PR that populates the field lands. That PR is ductone/c1#24250, held in draft for exactly this reason.

This and the connector.proto:131 thread are the same constraint stated at two widths; folded into the one note, at the wider width you have here, since the requirement is global rather than scoped to same-shape declarations. Leaving unresolved — nothing in the code changed for it.

Comment thread pkg/connectorbuilder/credential_issue_output_type_test.go

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@kans

kans commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Worth fixing (both trivial):

  1. credential_issue_validation.go:48-51 — the zero-match error
    discriminates on whether the REQUEST sent a type, not on why lookup
    failed. An unadvertised shape plus a supplied type reports "option X
    producing secret resource type Y is not advertised", implying the shape
    exists. Sends a connector author after the wrong problem.
  2. baton.proto:137 — comment claims empty "keeps the pre-existing
    shape-only selection". False once two descriptors share a shape, and it
    contradicts the accurate comment in resource.proto.

highb and others added 2 commits August 27, 2026 19:42
The zero-match error reported "credential option X producing secret
resource type Y is not advertised" whether the shape was unadvertised or
only the output type was, so an unadvertised shape read as though the
shape existed. Branch on the lookup failure instead: an unadvertised
shape says so on its own, and an unadvertised output type names the ones
that shape does produce.

Drop the decorative max_bytes rule on
CredentialIssueOptions.secret_resource_type_id and bound it in Go beside
the presence check. Nothing on the issuance path calls the generated
Validate(), which is the same reason min_bytes was never added.

Cover the case the tests could not distinguish: two API-key descriptors
that differ in advertised scopes, asserting the selected descriptor's own
constraints gate the request.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@c1-squire-dev

c1-squire-dev Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Squire (claude-opus-5): Both addressed — one fixed, one already gone.

1 · credential_issue_validation.go zero-match error — fixed in 4545eef0. The lookup now reports why it failed rather than what the request contained:

  • No descriptor advertises the shape at all → credential option API_KEY is not advertised by connector, with no mention of an output type, so it no longer implies the shape exists.
  • The shape is advertised but not producing that type → credential option API_KEY does not produce secret resource type "made-up-type"; it produces "organization-api-key", "service-account-application-key" — naming what the shape does produce, so the author has the answer rather than the next question.

Both cases are pinned by tests, including a negative assertion that the unadvertised-shape error does not name an output type.

2 · baton.proto:137 — the comment is already gone, and so is the field it described. Task.IssueCredentialTask.secret_resource_type_id = 5 was removed in 0873c7c5 when the discriminator moved onto CredentialIssueOptions; the task already carries credential_options = 2, so there was nothing left to forward. proto/c1/connectorapi/baton/v1/baton.proto is byte-identical to main. Deleted rather than reworded, which is the outcome you were pointing at — the review predates that commit.

Head is now b259a2c4. Same pass also folded in three CI-judge suggestions: dropped the decorative max_bytes on the new resource.proto field and bounded it in Go instead, added a same-shape-differing-scopes test permutation, and recorded the vendoring-order constraint in the PR body.

Comment thread pkg/connectorbuilder/credential_issue_validation.go
Comment thread proto/c1/connector/v2/resource.proto

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

… registration

The request-side length check had no registration-side counterpart, so a
connector advertising an over-long output type registered cleanly and then
failed every request naming it. Mirror the bound beside the existing
presence check, where the rest of the descriptor's validation fails fast.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Comment thread pkg/connectorbuilder/credential_issue_validation.go Outdated

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

The bound is mirrored in the proto on the descriptor and absent on the
request, so "rather than in the proto" described only half of it.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@linear-code

linear-code Bot commented Aug 27, 2026

Copy link
Copy Markdown

IGA-4107

highb and others added 2 commits August 27, 2026 21:59
preferred_option picks a credential shape, which stopped naming a single
descriptor once two can share one. Add bool preferred on the descriptor so
the two together always resolve to one default for a caller presenting a
choice.

Registration requires exactly one preferred descriptor per shape that has
several, and at most one per shape otherwise. A declaration that needs the
flag is one that could not register before this branch, so nothing that
registers today has to change. The flag selects nothing at issue time --
credential_options.secret_resource_type_id is still required.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
@c1-squire-dev

c1-squire-dev Bot commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Squire (claude-opus-5): bool preferred is in as of 60f3e133 — the deferral in the previous body is reversed, because a UI is now being built against these capabilities and needs a default selected in the picker.

Selection is two-level and both levels ship here: preferred_option picks the shape, CredentialIssueOptionDescriptor.preferred (field 10) picks the descriptor within it. Together they always resolve to exactly one default.

Registration enforces that rather than documenting it — exactly one preferred descriptor per shape that has several, at most one per shape otherwise. The two new rejections cost no existing connector anything: a declaration with two descriptors sharing a shape is one main rejects outright, so everything that registers today is unaffected.

preferred selects nothing at issue time. credential_options.secret_resource_type_id is still required on every request and resolution is still an exact lookup, with a test pinning that an empty output type fails even when a preferred descriptor exists — making the flag a fallback would reintroduce the implicit selection this PR removed.

Not deprecating preferred_option: it shipped in v0.25.1, is required at registration, and now has a well-defined narrower job that composes with the flag. No exported Go helper resolving the pair either — the consumer is c1's capability-to-offering mapping, so it would ship with no in-repo caller, and unlike the proto field a Go function is additive whenever it is added.

Body section rewritten from "open design question" to the resolved contract, with the registration table.

Comment thread proto/c1/connector/v2/connector.proto

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

highb and others added 2 commits August 27, 2026 22:11
preferred is a proto3 scalar with no presence and is only required where
several descriptors share a shape, so on every connector shipping today the
default descriptor has it unset. A consumer looking only for the flag finds
nothing in the common case; one scanning the whole list can land outside the
preferred shape.

Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

@highb
highb merged commit ef198a4 into main Aug 28, 2026
12 checks passed
@highb
highb deleted the highb/credential-issuer-output-type branch August 28, 2026 22:52
// must whenever several descriptors share that option: a default taken from
// declaration order would not be stable. It selects nothing at issue time --
// CredentialIssueOptions.secret_resource_type_id is still required.
bool preferred = 10;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use the order of the CredentialIssueOptionDescriptor list in CredentialDetailsCredentialIssue as a priority list? Then there's no chance of violating the contract.

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.

3 participants