[highb]

Squire (together/deepseek-ai/DeepSeek-V4.1-Flash-1017): A connector needs to declare what type of credential IssueCredential returns, because C1 binds that type before issuance and the connector cannot set it afterwards.

CredentialIssueOptionDescriptor.output (field 11) is a c1.config.v1.Field. The connector declares the returned credential's MIME type in content_type and its classification in tags, and C1 reads both from the descriptor the request selects (option plus secret_resource_type_id). It reuses #1213's Field.content_type (8) and Field.tags (9) rather than adding new fields.

Meaningful members are name (the returned PlaintextData.name), string_field (the value is a string), is_secret, content_type (decides the type) and tags (informational). Other members are ignored and should be left unset.

The Multipass payload helpers — the per-type structs, the encoder and the decoder — live in ConductorOne/baton-multipass-sdk (v0.0.1) rather than here. That repo is private for now.

Test plan

• go test ./pb/c1/connector/v2/ -run TestCredentialIssueOptionDescriptorOutput — a descriptor carrying application/vnd.multipass.api_key_v2+json with tags credential, multipass, api_key_v2 round-trips through the wire format and passes Validate(); content_type: "nope" fails on the ^[^/]+/[^/]+$ pattern.
• buf lint, golangci-lint v2.14.0 as CI pins it: 0 issues.

[linear-code]

IGA-5308

[github-actions]

Superseded — see the current review report for commit 4c7962f2d9e5

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a2c62f94e4e2.
Review mode: full
View review run

Review Summary

This PR adds an optional c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, imports c1/config/v1/config.proto into connector.proto, and regenerates connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go. The validator now recurses into Output, so the existing Field.content_type and Field.tags rules apply. Two tests cover the wire round-trip and rejection of a bad MIME type.

I scanned the full diff for security and correctness and applied the trusted repo-local criteria:

• Proto/wire: field 11 was not used before in this message, and nothing is renumbered or removed.
• Generated code: it matches the proto source in both the open and opaque APIs, including the descriptor bytes and the dep-index shift.
• Import cycle: none. pb/c1/config/v1 does not import connector/v2.
• Defaults and dependencies: there are no go.mod changes and no changes to default behavior.

Risk triage:

• Silence: no. The field is additive and optional, and an older reader just ignores it.
• Durability: yes. It is a wire field in capabilities metadata.
• Uncontrolled dimensions: no.
• Consumer distance: yes. C1 reads it.

Verdict: MEDIUM-LOW. The change is additive, wire-safe and not on a cost-contract path, so no escalation is needed. One gap: nothing in the SDK enforces that IssueCredential actually returns the declared type. The PR says this is out of scope.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence), proto/c1/connector/v2/connector.proto:151: output reuses the whole Field message, but only content_type/tags are meaningful here.
  • The comment doesn't say what name, is_required, is_secret or the field oneof mean in this position.
  • Field.content_type is documented as valid only for String/StringSlice/StringMap fields, which contradicts a descriptor with no oneof set (as in the test).
  • Please document that the other members are ignored and must be left unset, so C1 doesn't later have to interpret fields connectors populated ad hoc.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: The new `c1.config.v1.Field output = 11` reuses the full Field message, but only content_type and tags are meaningful for a credential issue descriptor. Extend the field comment to say that the other Field members (name, display_name, is_required, is_secret, the `field` oneof, etc.) are ignored and should be left unset (or name the expected oneof if one is intended). This resolves the conflict with Field.content_type's "Only valid for StringField, StringSliceField, & StringMapField" note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go so the Go doc comments match.

[github-actions]

Superseded — see the current review report for commit d483c04a2873

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a2c62f94e4e2.
Review mode: full
View review run

Review Summary

This PR adds an optional c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, imports c1/config/v1/config.proto into connector.proto, and regenerates connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go. The validator now recurses into Output, so the existing Field.content_type and Field.tags rules apply. Two tests cover the wire round-trip and rejection of a bad MIME type.

I scanned the full diff for security and correctness and applied the trusted repo-local criteria:

• Proto/wire: field 11 was not used before in this message, and nothing is renumbered or removed.
• Generated code: it matches the proto source in both the open and opaque APIs, including the descriptor bytes and the dep-index shift.
• Import cycle: none. pb/c1/config/v1 does not import connector/v2.
• Defaults and dependencies: there are no go.mod changes and no changes to default behavior.

Risk triage:

• Silence: no. The field is additive and optional, and an older reader just ignores it.
• Durability: yes. It is a wire field in capabilities metadata.
• Uncontrolled dimensions: no.
• Consumer distance: yes. C1 reads it.

Verdict: MEDIUM-LOW. The change is additive, wire-safe and not on a cost-contract path, so no escalation is needed. One gap: nothing in the SDK enforces that IssueCredential actually returns the declared type. The PR says this is out of scope.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence), proto/c1/connector/v2/connector.proto:151: output reuses the whole Field message, but only content_type/tags are meaningful here.
  • The comment doesn't say what name, is_required, is_secret or the field oneof mean in this position.
  • Field.content_type is documented as valid only for String/StringSlice/StringMap fields, which contradicts a descriptor with no oneof set (as in the test).
  • Please document that the other members are ignored and must be left unset, so C1 doesn't later have to interpret fields connectors populated ad hoc.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: The new `c1.config.v1.Field output = 11` reuses the full Field message, but only content_type and tags are meaningful for a credential issue descriptor. Extend the field comment to say that the other Field members (name, display_name, is_required, is_secret, the `field` oneof, etc.) are ignored and should be left unset (or name the expected oneof if one is intended). This resolves the conflict with Field.content_type's "Only valid for StringField, StringSliceField, & StringMapField" note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go so the Go doc comments match.

---

Reviewed commit: 4c7962f2d9e5

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit d483c04a2873

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a2c62f94e4e2.
Review mode: incremental since 4c7962f
View review run

Review Summary

This PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, with regenerated pb output and a validate/round-trip test. Since the last review, the new commits add a pkg/secret package: JSON structs for five Multipass types, ContentType/TypeIDFromContentType/Tags, OutputField, and Plaintext, which sets PlaintextData.schema. I confirmed the age, JWE and JWK providers copy that schema into EncryptedData. I scanned the full PR diff for security and correctness and found no secrets, injection or nil-safety problems. The repo criteria were applied:

• Wire compatibility: the proto change only adds a new field number (11). The generated .pb.go and .pb.validate.go changes match it.
• Go API: no exported API changes. pkg/secret is new and purely additive.
• Dependencies and defaults: no go.mod changes, and no default behavior changes.
• Risk triage:
  • Silence: low. An unset output is a visible absence for C1 to handle.
  • Durability: yes, but additive. It is a proto wire field.
  • Uncontrolled dimensions: no.
  • Consumer distance: the C1 platform.
  • Remediation: rung 1–2.
  • Verdict: not HIGH.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (high confidence): description doesn't match the code at pkg/secret/typeids.go:7-13. The PR body says hotp, note and generic "get type-ID constants only", but none are defined. Connectors issuing those types would have to hard-code the strings.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output field reuses the whole Field message, but its comment doesn't say that only content_type/tags matter here. It also conflicts with Field.content_type's note "Only valid for StringField, StringSliceField, & StringMapField". Meanwhile the test (and secret.OutputField) build an output with no oneof set. This was raised in the previous summary only; no new thread was opened.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 7-13: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Add `TypeHOTP = "hotp"`, `TypeNote = "note"`, `TypeGeneric = "generic"` with a comment that they use the UTF8_TEXT_V1 codec (so no struct and no ContentType JSON suffix applies), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

[github-actions]

Superseded — see the current review report for commit 101e5eed424c

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a2c62f94e4e2.
Review mode: incremental since 4c7962f
View review run

Review Summary

This PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, with regenerated pb output and a validate/round-trip test. Since the last review, the new commits add a pkg/secret package: JSON structs for five Multipass types, ContentType/TypeIDFromContentType/Tags, OutputField, and Plaintext, which sets PlaintextData.schema. I confirmed the age, JWE and JWK providers copy that schema into EncryptedData. I scanned the full PR diff for security and correctness and found no secrets, injection or nil-safety problems. The repo criteria were applied:

• Wire compatibility: the proto change only adds a new field number (11). The generated .pb.go and .pb.validate.go changes match it.
• Go API: no exported API changes. pkg/secret is new and purely additive.
• Dependencies and defaults: no go.mod changes, and no default behavior changes.
• Risk triage:
  • Silence: low. An unset output is a visible absence for C1 to handle.
  • Durability: yes, but additive. It is a proto wire field.
  • Uncontrolled dimensions: no.
  • Consumer distance: the C1 platform.
  • Remediation: rung 1–2.
  • Verdict: not HIGH.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (high confidence): description doesn't match the code at pkg/secret/typeids.go:7-13. The PR body says hotp, note and generic "get type-ID constants only", but none are defined. Connectors issuing those types would have to hard-code the strings.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output field reuses the whole Field message, but its comment doesn't say that only content_type/tags matter here. It also conflicts with Field.content_type's note "Only valid for StringField, StringSliceField, & StringMapField". Meanwhile the test (and secret.OutputField) build an output with no oneof set. This was raised in the previous summary only; no new thread was opened.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 7-13: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Add `TypeHOTP = "hotp"`, `TypeNote = "note"`, `TypeGeneric = "generic"` with a comment that they use the UTF8_TEXT_V1 codec (so no struct and no ContentType JSON suffix applies), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

---

Reviewed commit: d483c04a2873

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 101e5eed424c

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds CredentialIssueOptionDescriptor.output (field 11, a c1.config.v1.Field) with regenerated pb/validate code, plus a new additive pkg/secret package of Multipass serializers and helpers (ContentType, TypeIDFromContentType, Tags, OutputField, Plaintext). The only new commit since the last review (101e5ee) adds a //nolint:gosec comment to the OAuthClientSecretV2 test fixture in pkg/secret/encode_test.go:71. That is a lint suppression on fake test values and changes no behavior. I rescanned the full PR diff for security and correctness and found nothing new. Repo-local criteria triage is unchanged and LOW: the change is purely additive (new proto field number, no renumbering or reuse, generated code matches), no exported API is broken, defaults and serialized pagination state are untouched, and go.mod did not change. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (high confidence): pkg/secret/typeids.go:7-13. The PR body says hotp, note and generic "get type-ID constants only", but the const block defines only the five JSON types. The existing open thread covers this.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output comment doesn't say that only content_type/tags (and is_secret/name) are read and that the field oneof should be left unset. It also conflicts with Field.content_type's note "Only valid for StringField, StringSliceField, & StringMapField". This has been raised in the summary only.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 7-13: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Either add `TypeHOTP = "hotp"`, `TypeNote = "note"`, `TypeGeneric = "generic"` with a comment that they use the UTF8_TEXT_V1 codec (so no struct applies and the +json ContentType does not apply), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

[github-actions]

Superseded — see the current review report for commit c5901b23175f

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds CredentialIssueOptionDescriptor.output (field 11, a c1.config.v1.Field) with regenerated pb/validate code, plus a new additive pkg/secret package of Multipass serializers and helpers (ContentType, TypeIDFromContentType, Tags, OutputField, Plaintext). The only new commit since the last review (101e5ee) adds a //nolint:gosec comment to the OAuthClientSecretV2 test fixture in pkg/secret/encode_test.go:71. That is a lint suppression on fake test values and changes no behavior. I rescanned the full PR diff for security and correctness and found nothing new. Repo-local criteria triage is unchanged and LOW: the change is purely additive (new proto field number, no renumbering or reuse, generated code matches), no exported API is broken, defaults and serialized pagination state are untouched, and go.mod did not change. The incremental diff was complete (not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (high confidence): pkg/secret/typeids.go:7-13. The PR body says hotp, note and generic "get type-ID constants only", but the const block defines only the five JSON types. The existing open thread covers this.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output comment doesn't say that only content_type/tags (and is_secret/name) are read and that the field oneof should be left unset. It also conflicts with Field.content_type's note "Only valid for StringField, StringSliceField, & StringMapField". This has been raised in the summary only.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 7-13: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Either add `TypeHOTP = "hotp"`, `TypeNote = "note"`, `TypeGeneric = "generic"` with a comment that they use the UTF8_TEXT_V1 codec (so no struct applies and the +json ContentType does not apply), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

---

Reviewed commit: 101e5eed424c

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit c5901b23175f

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 6b910a95f0f4.
Review mode: full
View review run

Review Summary

The PR adds an optional c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor. It regenerates the .pb.go, opaque and validate output to match, so Validate() now recurses into Output. It also adds a new pkg/secret package: JSON structs for five Multipass types plus the ContentType, TypeIDFromContentType, Tags, OutputField and Plaintext helpers. The latest commit switches Tags to pkg/field's FieldTag constants.

I scanned the full diff for security and correctness and found no issues. There is no import cycle: pb/c1/config/v1 does not import connector/v2. The four tags Tags returns fit Field.tags validation (at most 16 items, pattern ^[a-zA-Z0-9_.-]+$), and every ContentType(id) value for the known IDs matches ^[^/]+/[^/]+$.

I applied the repo criteria:

• Triage:
  • Silence: yes. A wrong content type would be well-formed and bind the wrong type with no error.
  • Durability: yes. It is a proto wire field.
  • Uncontrolled dimensions: no.
  • Consumer distance: yes. The C1 platform reads it.
  • Remediation: rung 1–2. The field is purely additive and optional, the default path is unchanged, and older SDKs simply omit field 11.
  • Verdict: MEDIUM. Additive, opt-in and not version-pair dependent, so no escalation.
• Proto: new field number, nothing renumbered or reused, generated code regenerated.
• Exported API: new package only, no interface changes.
• Dependencies: go.mod and go.sum are unchanged.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (high confidence): pkg/secret/typeids.go:11-17. The PR body says hotp, note and generic "get type-ID constants only", but the const block still defines only the five JSON types. The existing open thread covers this.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output comment doesn't say that only content_type/tags (and is_secret/name) are read, or that the field oneof should be left unset. That clashes with Field.content_type's "Only valid for StringField, StringSliceField, & StringMapField" note (proto/c1/config/v1/config.proto:63). This was raised in the summary only; no new thread.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 11-17: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Either add `TypeHOTP = "hotp"`, `TypeNote = "note"` and `TypeGeneric = "generic"`, with a comment that they use the UTF8_TEXT_V1 codec (so no struct applies and the +json ContentType does not apply), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

[github-actions]

Superseded — see the current review report for commit 61f186d39069

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 6b910a95f0f4.
Review mode: full
View review run

Review Summary

The PR adds an optional c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor. It regenerates the .pb.go, opaque and validate output to match, so Validate() now recurses into Output. It also adds a new pkg/secret package: JSON structs for five Multipass types plus the ContentType, TypeIDFromContentType, Tags, OutputField and Plaintext helpers. The latest commit switches Tags to pkg/field's FieldTag constants.

I scanned the full diff for security and correctness and found no issues. There is no import cycle: pb/c1/config/v1 does not import connector/v2. The four tags Tags returns fit Field.tags validation (at most 16 items, pattern ^[a-zA-Z0-9_.-]+$), and every ContentType(id) value for the known IDs matches ^[^/]+/[^/]+$.

I applied the repo criteria:

• Triage:
  • Silence: yes. A wrong content type would be well-formed and bind the wrong type with no error.
  • Durability: yes. It is a proto wire field.
  • Uncontrolled dimensions: no.
  • Consumer distance: yes. The C1 platform reads it.
  • Remediation: rung 1–2. The field is purely additive and optional, the default path is unchanged, and older SDKs simply omit field 11.
  • Verdict: MEDIUM. Additive, opt-in and not version-pair dependent, so no escalation.
• Proto: new field number, nothing renumbered or reused, generated code regenerated.
• Exported API: new package only, no interface changes.
• Dependencies: go.mod and go.sum are unchanged.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (high confidence): pkg/secret/typeids.go:11-17. The PR body says hotp, note and generic "get type-ID constants only", but the const block still defines only the five JSON types. The existing open thread covers this.
• Prior — still present (medium confidence): proto/c1/connector/v2/connector.proto:149-151. The output comment doesn't say that only content_type/tags (and is_secret/name) are read, or that the field oneof should be left unset. That clashes with Field.content_type's "Only valid for StringField, StringSliceField, & StringMapField" note (proto/c1/config/v1/config.proto:63). This was raised in the summary only; no new thread.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/secret/typeids.go`:
- Around line 11-17: The PR description says hotp, note and generic get type-ID constants, but the const block only has the five JSON types. Either add `TypeHOTP = "hotp"`, `TypeNote = "note"` and `TypeGeneric = "generic"`, with a comment that they use the UTF8_TEXT_V1 codec (so no struct applies and the +json ContentType does not apply), or remove that claim from the PR description.

In `proto/c1/connector/v2/connector.proto`:
- Around line 149-151: Extend the comment on `c1.config.v1.Field output = 11`. Say that only content_type and tags (and optionally is_secret/name) are read, and that the `field` oneof and the other Field members are ignored and should be left unset. This resolves the conflict with Field.content_type's "Only valid for StringField..." note. Then regenerate pb/c1/connector/v2/connector.pb.go and connector_protoopaque.pb.go.

---

Reviewed commit: c5901b23175f

[github-actions]

No blocking issues found — see the full review report

[github-actions]

General PR Review:

⏳ Provisional — deeper review still in progress.

Review in progress; this comment will be replaced with the full summary.

[github-actions]

Superseded — see the current review report for commit 61f186d39069

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds CredentialIssueOptionDescriptor.output (field 11, a c1.config.v1.Field) and its generated Go code. It also adds a new pkg/secret package with JSON serializers for five Multipass types, plus the ContentType, TypeIDFromContentType, Tags, OutputField and Plaintext helpers.

The new commits since c5901b2 do six things:

• OutputField now sets an empty StringField to satisfy the content_type constraint.
• Plaintext wraps its encode error with %w and has a test for that.
• TagMultipass is unexported. pkg/secret is new in this PR, so this breaks no existing caller.
• ContentType and APIKeyV2.ExpiresAt get doc comments.
• The proto comment on output now says which Field members are read.
• The PR body now says only the five JSON types get constants.

I scanned the full PR diff for security and correctness. The incremental artifact left out connector.pb.go and connector_protoopaque.pb.go. I checked their hunks in the full diff: they add Output, GetOutput, SetOutput, HasOutput/ClearOutput, the v1 import, and the regenerated descriptor. These match the proto change, and pb/c1/config/v1 does not import connector/v2, so there is no import cycle. I did not decode the raw descriptor bytes byte by byte.

Criteria applied (risk triage):

• Silence: partial. A wrong content_type would make C1 bind the wrong type, but C1 validates against Multipass.
• Durability: yes. This is a proto wire field.
• Uncontrolled dimensions: no.
• Consumer distance: yes. The C1 platform reads it.

Verdict: MEDIUM, not HIGH. The proto change is purely additive: a new field number, nothing renumbered or reused, and generated code regenerated alongside it. The field is optional and nil by default, and validation only runs when it is set, so existing connectors behave the same.

None of these changed: go.mod/go.sum, pkg/connectorbuilder interfaces, pagination or serialized state, and pkg/sdk/version.go.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• pkg/secret/typeids.go:17 (hotp/note/generic constants claim): fixed. The PR body now says "Only the five JSON types connectors emit get constants", and pkg/secret/typeids.go:9 scopes the const block to "Content type IDs a connector can emit".
• proto/c1/connector/v2/connector.proto:149-151 (summary-only; output comment didn't say which members are read): fixed. connector.proto:149-153 now lists the members that are read (name, string_field, is_secret, content_type, tags) and says the others should be left unset, and OutputField sets StringField (pkg/secret/encode.go:20). The generated Go comments are regenerated to match.

[github-actions]

Superseded — see the current review report for commit dc065ae56b48

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds CredentialIssueOptionDescriptor.output (field 11, a c1.config.v1.Field) and its generated Go code. It also adds a new pkg/secret package with JSON serializers for five Multipass types, plus the ContentType, TypeIDFromContentType, Tags, OutputField and Plaintext helpers.

The new commits since c5901b2 do six things:

• OutputField now sets an empty StringField to satisfy the content_type constraint.
• Plaintext wraps its encode error with %w and has a test for that.
• TagMultipass is unexported. pkg/secret is new in this PR, so this breaks no existing caller.
• ContentType and APIKeyV2.ExpiresAt get doc comments.
• The proto comment on output now says which Field members are read.
• The PR body now says only the five JSON types get constants.

I scanned the full PR diff for security and correctness. The incremental artifact left out connector.pb.go and connector_protoopaque.pb.go. I checked their hunks in the full diff: they add Output, GetOutput, SetOutput, HasOutput/ClearOutput, the v1 import, and the regenerated descriptor. These match the proto change, and pb/c1/config/v1 does not import connector/v2, so there is no import cycle. I did not decode the raw descriptor bytes byte by byte.

Criteria applied (risk triage):

• Silence: partial. A wrong content_type would make C1 bind the wrong type, but C1 validates against Multipass.
• Durability: yes. This is a proto wire field.
• Uncontrolled dimensions: no.
• Consumer distance: yes. The C1 platform reads it.

Verdict: MEDIUM, not HIGH. The proto change is purely additive: a new field number, nothing renumbered or reused, and generated code regenerated alongside it. The field is optional and nil by default, and validation only runs when it is set, so existing connectors behave the same.

None of these changed: go.mod/go.sum, pkg/connectorbuilder interfaces, pagination or serialized state, and pkg/sdk/version.go.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• pkg/secret/typeids.go:17 (hotp/note/generic constants claim): fixed. The PR body now says "Only the five JSON types connectors emit get constants", and pkg/secret/typeids.go:9 scopes the const block to "Content type IDs a connector can emit".
• proto/c1/connector/v2/connector.proto:149-151 (summary-only; output comment didn't say which members are read): fixed. connector.proto:149-153 now lists the members that are read (name, string_field, is_secret, content_type, tags) and says the others should be left unset, and OutputField sets StringField (pkg/secret/encode.go:20). The generated Go comments are regenerated to match.

---

Reviewed commit: 61f186d39069

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit dc065ae56b48

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 6b910a95f0f4.
Review mode: full
View review run

Review Summary

The PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, which imports config.proto into connector.proto. It includes regenerated connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go. The validator now runs Field's content_type and tags rules on the embedded message. Two tests cover the wire round-trip and rejection of an invalid content type. The earlier pkg/secret helpers were removed, and the PR body now says they live in a separate library. I scanned the full diff for security and correctness issues.

I applied the repo criteria as follows:

• Proto/wire: field 11 is new and was not reserved or used before. This is an additive message field, and the generated code matches the proto source.
• Import cycle: pb/c1/config/v1 imports nothing from connector/v2, so there is no cycle.
• Exported Go API: only additive getters, setters and builder fields.
• Defaults: unchanged. An unset output validates as before.
• Dependencies: no go.mod or go.sum changes.

Risk triage:

• Silence: partial. A wrong content_type binds the wrong type in C1.
• Durability: yes, because this is a wire field. It is additive, so remediation is rung 1–2.
• Uncontrolled dimensions: no.
• Consumer distance: yes, since the C1 platform reads the field.

Verdict: MEDIUM. Older readers skip the unknown field 11. A new SDK reading old descriptors sees output == nil. Neither direction depends on a version pair in a way that breaks.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence): pb/c1/connector/v2/credential_issue_output_validate_test.go:18-21. The only example of a populated output sets content_type without string_field. config.proto:63 says content_type is "Only valid for StringField…", and Validate() doesn't enforce that pairing. Connectors that copy this fixture would ship descriptors with no type member.

Resolved prior findings

• pkg/secret/typeids.go (type-ID constants vs. PR body claim): obsolete. pkg/secret was removed from this PR, and the payload helpers now live in a separate library according to the PR body. The thread was already human-resolved.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pb/c1/connector/v2/credential_issue_output_validate_test.go`:
- Around line 18-21: The round-trip fixture builds `config.Field_builder{ContentType: ..., Tags: ...}` without a `StringField`, yet config.proto says content_type is only valid for string fields, and the new `output` comment lists string_field as meaningful. Add `StringField: &config.StringField{}` (or the builder equivalent) to the fixture so the canonical example declares a string-valued output.

[github-actions]

Superseded — see the current review report for commit fd44e42a7438

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 6b910a95f0f4.
Review mode: full
View review run

Review Summary

The PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor, which imports config.proto into connector.proto. It includes regenerated connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go. The validator now runs Field's content_type and tags rules on the embedded message. Two tests cover the wire round-trip and rejection of an invalid content type. The earlier pkg/secret helpers were removed, and the PR body now says they live in a separate library. I scanned the full diff for security and correctness issues.

I applied the repo criteria as follows:

• Proto/wire: field 11 is new and was not reserved or used before. This is an additive message field, and the generated code matches the proto source.
• Import cycle: pb/c1/config/v1 imports nothing from connector/v2, so there is no cycle.
• Exported Go API: only additive getters, setters and builder fields.
• Defaults: unchanged. An unset output validates as before.
• Dependencies: no go.mod or go.sum changes.

Risk triage:

• Silence: partial. A wrong content_type binds the wrong type in C1.
• Durability: yes, because this is a wire field. It is additive, so remediation is rung 1–2.
• Uncontrolled dimensions: no.
• Consumer distance: yes, since the C1 platform reads the field.

Verdict: MEDIUM. Older readers skip the unknown field 11. A new SDK reading old descriptors sees output == nil. Neither direction depends on a version pair in a way that breaks.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence): pb/c1/connector/v2/credential_issue_output_validate_test.go:18-21. The only example of a populated output sets content_type without string_field. config.proto:63 says content_type is "Only valid for StringField…", and Validate() doesn't enforce that pairing. Connectors that copy this fixture would ship descriptors with no type member.

Resolved prior findings

• pkg/secret/typeids.go (type-ID constants vs. PR body claim): obsolete. pkg/secret was removed from this PR, and the payload helpers now live in a separate library according to the PR body. The thread was already human-resolved.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pb/c1/connector/v2/credential_issue_output_validate_test.go`:
- Around line 18-21: The round-trip fixture builds `config.Field_builder{ContentType: ..., Tags: ...}` without a `StringField`, yet config.proto says content_type is only valid for string fields, and the new `output` comment lists string_field as meaningful. Add `StringField: &config.StringField{}` (or the builder equivalent) to the fixture so the canonical example declares a string-valued output.

---

Reviewed commit: dc065ae56b48

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit fd44e42a7438

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor (proto/c1/connector/v2/connector.proto:154). It regenerates connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go to match, and the validator now recurses into Output. The new commit sets StringField in the round-trip fixture and checks that the oneof survives the wire round trip. That resolves the suggestion from the last review.

I scanned the full PR diff for security and correctness. I applied the repo-local criteria like this:

• Wire compatibility: field 11 is new and isn't reserved or reused, and no existing field changed. The generated code matches the proto.
• Defaults: an unset output validates the same as before.
• Dependencies: go.mod and go.sum are unchanged.
• Risk triage: the change is additive and opt-in, so the escape risk is LOW. A wrong content_type that a connector declares is that connector's own data, not an SDK default.

The incremental diff was complete (partial: false).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• pkg/secret/typeids.go (type-ID constants vs. the PR body): obsolete. pkg/secret is no longer in this PR, and the payload helpers moved to baton-multipass-sdk.
• Round-trip fixture missing string_field (raised in the summary last time): fixed at pb/c1/connector/v2/credential_issue_output_validate_test.go:20, which adds StringField, and line 31, which asserts HasStringField() after unmarshal.

[github-actions]

General PR Review: IGA-5308: Declare typed output on credential issue descriptors

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

Review Summary

The PR adds c1.config.v1.Field output = 11 to CredentialIssueOptionDescriptor (proto/c1/connector/v2/connector.proto:154). It regenerates connector.pb.go, connector_protoopaque.pb.go and connector.pb.validate.go to match, and the validator now recurses into Output. The new commit sets StringField in the round-trip fixture and checks that the oneof survives the wire round trip. That resolves the suggestion from the last review.

I scanned the full PR diff for security and correctness. I applied the repo-local criteria like this:

• Wire compatibility: field 11 is new and isn't reserved or reused, and no existing field changed. The generated code matches the proto.
• Defaults: an unset output validates the same as before.
• Dependencies: go.mod and go.sum are unchanged.
• Risk triage: the change is additive and opt-in, so the escape risk is LOW. A wrong content_type that a connector declares is that connector's own data, not an SDK default.

The incremental diff was complete (partial: false).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• pkg/secret/typeids.go (type-ID constants vs. the PR body): obsolete. pkg/secret is no longer in this PR, and the payload helpers moved to baton-multipass-sdk.
• Round-trip fixture missing string_field (raised in the summary last time): fixed at pb/c1/connector/v2/credential_issue_output_validate_test.go:20, which adds StringField, and line 31, which asserts HasStringField() after unmarshal.

---

Reviewed commit: fd44e42a7438

[github-actions]

No blocking issues found — see the full review report