[highb]

Squire (openai/gpt-6.1-sol):

Let connectors declare a secret output's content type before producer execution. Existing generic declarations remain compatible. Registration snapshots the declaration and refuses incompatible typed-output cardinality; generic/legacy multi-secret and typed-singleton-plus-public-result cases remain supported.

For explicitly typed output, the existing full-knowledge recipient context must agree before action allocation or handler invocation. Result metadata cannot override the snapshotted declaration. These checks bind delivery identity and encryption context; they do not validate payload JSON fields or a Multipass schema.

Review this bottom layer first, then #1194 for simple credential JSON builders and #1195 for native request inputs and output binding. The native tip is the single combined C1 dependency. No Multipass contract-export PR or copied schema bundle is required.

This output-layer source is unchanged by the builder simplification. Its existing registration, snapshot, context, encryption/opening and generic compatibility results retain their original revision attribution. New builder/native tests and compatibility checks are recorded on the successor PRs. Historical broader normal/race/build/vet checks and the unrelated local uhttp lint baseline remain attributed to their original revisions; no baseline waiver is implied.

Published history is preserved through forward merges. No merge, SDK release, deployment, live provider or permission expansion is claimed.

[github-actions]

Superseded — see the current review report for commit da275f138a7e

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional proto field, config.Field.secret_content_type = 8, with PGV rules (length 1–128, a lowercase pattern) and checked-in regenerated pb output. Registration now refuses a declaration that is on an argument, on a public return, empty, or malformed. Each handler stores the declared types as a map when it registers. Before an action is allocated or its handler runs, invokeRegisteredAction (pkg/actions/actions.go:927) checks that every JWE recipient carries the AAD {"purpose":"c1/fk-credential-delivery","version":1,"content_type":<declared>}, and it rejects duplicate JSON keys. Undeclared or generic outputs skip this check, so they behave as before. I scanned the full PR diff for security and correctness and found no blocking issues.

Repo-local criteria:

• Triage:
  • Silence: low. A mismatch is refused loudly with FailedPrecondition.
  • Durability: yes. The AAD is bound into the encrypted output that C1 stores.
  • Version-pair dependence: yes. This is an old C1 caller against a new connector.
  • Consumer distance: yes. The consumer is the C1 platform.
  • Verdict: HIGH (version-pair). The risk falls in the review-blind multi-artifact / version-pair class. The PR does include the instrument: a permutation table of missing/opaque/generic/wrong/duplicate/invalid contexts for both global and resource actions, which asserts that the handler call count and the action ID stay zero. It also reports a mutation check showing the regression test fails when the guard is removed. The cross-repo C1 integration is explicitly deferred, and I recommend escalating to that integration stage.
• Proto: the change is additive and reuses no field numbers, and the generated code matches the source.
• Exported API: only additive generated accessors were added. No interface changes and no default-behavior changes.
• Dependencies and version: go.mod is unchanged, and validate was already a module dependency. Additive behavior does not require a bump to pkg/sdk/version.go.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence): pkg/actions/secret_content_type.go:24-33. An action with one typed secret return plus other secret returns registers successfully, but every invocation of it fails in validateTypedOutputContext ("requires exactly one secret return"; see the test at secret_content_type_test.go:183). Rejecting this at registration would surface the error when the connector starts.
• New (low confidence): docs/verification/connector-secret-types/{plan,evidence}.md. These files are process ledgers: the plan table still says "Evidence incomplete", and the evidence file cites pre-squash commit hashes (528830db, 9a550250) that will not resolve after merge. Consider keeping them in the PR description instead of in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: In validateActionSecretContentTypes, when any return field declares a
  secret_content_type other than "generic", count the secret return fields (IsSecret=true) and
  return an error if there is more than one. That moves the "typed secret action output requires
  exactly one secret return" failure from every invocation (validateTypedOutputContext) to
  registration time. Update the test at secret_content_type_test.go:183 to assert that
  registration fails instead.

In `docs/verification/connector-secret-types/plan.md` and `evidence.md`:
- These are process ledgers that contain stale "Evidence incomplete" statuses and pre-squash
  commit hashes. Either drop them from the repository (move the content to the PR description),
  or update the statuses and remove the commit-hash references so they stay accurate after merge.

[github-actions]

🟡 Suggestion (medium confidence): an action with one typed secret return plus any other secret return passes registration, but validateTypedOutputContext then fails every invocation with "requires exactly one secret return" (see the test at secret_content_type_test.go:183). Since registration already rejects other invalid declarations, it could reject this one too, so the connector author sees the error at startup rather than on the first production call.

[github-actions]

Superseded — see the current review report for commit 899cb2f0457c

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional proto field, config.Field.secret_content_type = 8, with PGV rules (length 1–128, a lowercase pattern) and checked-in regenerated pb output. Registration now refuses a declaration that is on an argument, on a public return, empty, or malformed. Each handler stores the declared types as a map when it registers. Before an action is allocated or its handler runs, invokeRegisteredAction (pkg/actions/actions.go:927) checks that every JWE recipient carries the AAD {"purpose":"c1/fk-credential-delivery","version":1,"content_type":<declared>}, and it rejects duplicate JSON keys. Undeclared or generic outputs skip this check, so they behave as before. I scanned the full PR diff for security and correctness and found no blocking issues.

Repo-local criteria:

• Triage:
  • Silence: low. A mismatch is refused loudly with FailedPrecondition.
  • Durability: yes. The AAD is bound into the encrypted output that C1 stores.
  • Version-pair dependence: yes. This is an old C1 caller against a new connector.
  • Consumer distance: yes. The consumer is the C1 platform.
  • Verdict: HIGH (version-pair). The risk falls in the review-blind multi-artifact / version-pair class. The PR does include the instrument: a permutation table of missing/opaque/generic/wrong/duplicate/invalid contexts for both global and resource actions, which asserts that the handler call count and the action ID stay zero. It also reports a mutation check showing the regression test fails when the guard is removed. The cross-repo C1 integration is explicitly deferred, and I recommend escalating to that integration stage.
• Proto: the change is additive and reuses no field numbers, and the generated code matches the source.
• Exported API: only additive generated accessors were added. No interface changes and no default-behavior changes.
• Dependencies and version: go.mod is unchanged, and validate was already a module dependency. Additive behavior does not require a bump to pkg/sdk/version.go.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence): pkg/actions/secret_content_type.go:24-33. An action with one typed secret return plus other secret returns registers successfully, but every invocation of it fails in validateTypedOutputContext ("requires exactly one secret return"; see the test at secret_content_type_test.go:183). Rejecting this at registration would surface the error when the connector starts.
• New (low confidence): docs/verification/connector-secret-types/{plan,evidence}.md. These files are process ledgers: the plan table still says "Evidence incomplete", and the evidence file cites pre-squash commit hashes (528830db, 9a550250) that will not resolve after merge. Consider keeping them in the PR description instead of in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: In validateActionSecretContentTypes, when any return field declares a
  secret_content_type other than "generic", count the secret return fields (IsSecret=true) and
  return an error if there is more than one. That moves the "typed secret action output requires
  exactly one secret return" failure from every invocation (validateTypedOutputContext) to
  registration time. Update the test at secret_content_type_test.go:183 to assert that
  registration fails instead.

In `docs/verification/connector-secret-types/plan.md` and `evidence.md`:
- These are process ledgers that contain stale "Evidence incomplete" statuses and pre-squash
  commit hashes. Either drop them from the repository (move the content to the PR description),
  or update the statuses and remove the commit-hash references so they stay accurate after merge.

---

Reviewed commit: da275f138a7e

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 899cb2f0457c

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The new commits close a gap between the advertised declaration and the enforced one. registerActionSchema and registerResourceAction now store proto.Clone copies of the schema, and ListActionSchemas and GetActionSchema return clones (pkg/actions/actions.go:443,643,713-731,746). A connector or caller that mutates a schema pointer can no longer change the advertised secret_content_type while the handler keeps enforcing its snapshot from registration. The commits also add TestSecretOutputAdvertisedSchemaIsAnImmutableSnapshot, which covers six global/resource × absent/generic/password_v2 cases, and update the docs ledgers. Proto and generated files are unchanged since the last review.

I scanned the full PR diff for security and correctness. A typed nil cannot reach the clones, because both registration paths reject a nil schema before cloning. The added cost is linear copying when schemas are registered or listed, which is not on a grant-scale or artifact-open path.

Criteria triage for this increment:

• Silence: no. A mismatch fails loudly with FailedPrecondition.
• Durability: no. The copies live only in memory.
• Uncontrolled dimensions: no.
• Consumer distance: platform discovery output, but the wire shape does not change.
• Verdict: LOW for this increment.

Callers that mutated returned schemas to change the registry would see a behavior change, but nothing in-tree relies on that. go.mod and pkg/sdk/version.go are unchanged, and the change is additive.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes checks each return field on its own. An action with one typed secret return plus other secret returns therefore still registers successfully, then fails on every invocation in validateTypedOutputContext (secret_content_type.go:62-64, "requires exactly one secret return"). Rejecting that shape at registration would surface the misconfiguration when the connector starts instead of on the first call.
• Prior — still present (low confidence): docs/verification/connector-secret-types/{plan,evidence}.md. The evidence ledger now also cites 4483426d, alongside the pre-squash hashes 528830db and 9a550250. None of these will resolve after a squash merge. These process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes accepts a schema where one secret return declares a non-generic secret_content_type while other secret returns also exist, but validateTypedOutputContext (line 62) refuses every invocation of such an action. Add a registration-time check: if any secret return has a non-"generic" secret_content_type, require that the schema has exactly one return with is_secret=true, and return an error otherwise. Update the test at secret_content_type_test.go (~line 183) to expect a registration error.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- These ledgers cite pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after merge. Consider moving this content into the PR description, or removing the commit references before merge.

[github-actions]

Superseded — see the current review report for commit b4c6a5b06b11

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The new commits close a gap between the advertised declaration and the enforced one. registerActionSchema and registerResourceAction now store proto.Clone copies of the schema, and ListActionSchemas and GetActionSchema return clones (pkg/actions/actions.go:443,643,713-731,746). A connector or caller that mutates a schema pointer can no longer change the advertised secret_content_type while the handler keeps enforcing its snapshot from registration. The commits also add TestSecretOutputAdvertisedSchemaIsAnImmutableSnapshot, which covers six global/resource × absent/generic/password_v2 cases, and update the docs ledgers. Proto and generated files are unchanged since the last review.

I scanned the full PR diff for security and correctness. A typed nil cannot reach the clones, because both registration paths reject a nil schema before cloning. The added cost is linear copying when schemas are registered or listed, which is not on a grant-scale or artifact-open path.

Criteria triage for this increment:

• Silence: no. A mismatch fails loudly with FailedPrecondition.
• Durability: no. The copies live only in memory.
• Uncontrolled dimensions: no.
• Consumer distance: platform discovery output, but the wire shape does not change.
• Verdict: LOW for this increment.

Callers that mutated returned schemas to change the registry would see a behavior change, but nothing in-tree relies on that. go.mod and pkg/sdk/version.go are unchanged, and the change is additive.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes checks each return field on its own. An action with one typed secret return plus other secret returns therefore still registers successfully, then fails on every invocation in validateTypedOutputContext (secret_content_type.go:62-64, "requires exactly one secret return"). Rejecting that shape at registration would surface the misconfiguration when the connector starts instead of on the first call.
• Prior — still present (low confidence): docs/verification/connector-secret-types/{plan,evidence}.md. The evidence ledger now also cites 4483426d, alongside the pre-squash hashes 528830db and 9a550250. None of these will resolve after a squash merge. These process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes accepts a schema where one secret return declares a non-generic secret_content_type while other secret returns also exist, but validateTypedOutputContext (line 62) refuses every invocation of such an action. Add a registration-time check: if any secret return has a non-"generic" secret_content_type, require that the schema has exactly one return with is_secret=true, and return an error otherwise. Update the test at secret_content_type_test.go (~line 183) to expect a registration error.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- These ledgers cite pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after merge. Consider moving this content into the PR description, or removing the commit references before merge.

---

Reviewed commit: 899cb2f0457c

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit b4c6a5b06b11

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional config.Field.secret_content_type (new field 8, optional, so presence is tracked). Bad declarations are rejected at registration, the declared types are copied when the action is registered, and stored schemas are cloned when they are read. Before the handler runs, the code checks that a typed secret output has a matching full-knowledge JWE recipient context. Since the last review, the only new commit (b4c6a5b0) is a lint cleanup in pkg/actions/secret_content_type_test.go: a //nolint:gosec on a constant fixture, and an if/else chain turned into a switch. Neither change affects behavior.

I scanned the full PR diff again for security and correctness: actions.go, secret_content_type.go, the proto, and the generated pb output. Field 8 does not collide with any other field in Field, and the generated code is included.

The repo-local criteria were applied:

• Wire compatibility: the change is additive and optional.
• Default behavior: absent means generic, so the default is unchanged.
• Exported Go API: no interface or signature changes.
• Triage: loud failure (FailedPrecondition), no durable state, low risk.

There is no incremental coverage gap (the artifact is not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes validates each return field on its own. An action with one typed secret return plus other secret returns therefore registers successfully. It then fails on every call in validateTypedOutputContext (secret_content_type.go:62-64, "requires exactly one secret return"). Rejecting that shape at registration would surface the mistake when the connector starts, not on the first call.
• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 (and plan.md). The ledger cites the pre-squash commit hashes 528830db, 9a550250 and 4483426d, none of which will resolve after a squash merge. These process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes accepts a schema where one secret return declares a non-generic secret_content_type while other secret returns also exist. validateTypedOutputContext (line 62-64) then rejects every invocation with "requires exactly one secret return". Add a registration-time check: if any return has a non-generic content type, require that exactly one return field has is_secret=true, otherwise return an error. Add a test covering registration rejection for this shape.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around line 3: The ledger references pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after squash merge. Consider moving these verification ledgers into the PR description, or dropping the commit hash references, rather than committing them under docs/.

[github-actions]

Superseded — see the current review report for commit c65a6c4b4c23

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional config.Field.secret_content_type (new field 8, optional, so presence is tracked). Bad declarations are rejected at registration, the declared types are copied when the action is registered, and stored schemas are cloned when they are read. Before the handler runs, the code checks that a typed secret output has a matching full-knowledge JWE recipient context. Since the last review, the only new commit (b4c6a5b0) is a lint cleanup in pkg/actions/secret_content_type_test.go: a //nolint:gosec on a constant fixture, and an if/else chain turned into a switch. Neither change affects behavior.

I scanned the full PR diff again for security and correctness: actions.go, secret_content_type.go, the proto, and the generated pb output. Field 8 does not collide with any other field in Field, and the generated code is included.

The repo-local criteria were applied:

• Wire compatibility: the change is additive and optional.
• Default behavior: absent means generic, so the default is unchanged.
• Exported Go API: no interface or signature changes.
• Triage: loud failure (FailedPrecondition), no durable state, low risk.

There is no incremental coverage gap (the artifact is not partial).

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes validates each return field on its own. An action with one typed secret return plus other secret returns therefore registers successfully. It then fails on every call in validateTypedOutputContext (secret_content_type.go:62-64, "requires exactly one secret return"). Rejecting that shape at registration would surface the mistake when the connector starts, not on the first call.
• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 (and plan.md). The ledger cites the pre-squash commit hashes 528830db, 9a550250 and 4483426d, none of which will resolve after a squash merge. These process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes accepts a schema where one secret return declares a non-generic secret_content_type while other secret returns also exist. validateTypedOutputContext (line 62-64) then rejects every invocation with "requires exactly one secret return". Add a registration-time check: if any return has a non-generic content type, require that exactly one return field has is_secret=true, otherwise return an error. Add a test covering registration rejection for this shape.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around line 3: The ledger references pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after squash merge. Consider moving these verification ledgers into the PR description, or dropping the commit hash references, rather than committing them under docs/.

---

Reviewed commit: b4c6a5b06b11

[github-actions]

No blocking issues found — see the full review report

[ggreer]

Why not make this content_type? There are probably non-secret fields that we'd like to hint at the content.

Also can a field have multiple possible content types? eg: Could an action return YAML or JSON depending on some configuration?

[github-actions]

Superseded — see the current review report for commit c65a6c4b4c23

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

This PR adds an optional proto field, config.Field.secret_content_type (field 8, with PGV rules). Its number is not reused and generated pb, pb.validate and protoopaque output is included. The field is validated at registration: it is refused on arguments, public returns, and empty or malformed values. The declaration is captured into registeredActionHandler.secretReturnContentTypes when the action is registered. A typed output is refused with FailedPrecondition unless the single JWE recipient's AAD declares the matching c1/fk-credential-delivery v1 content type. That check runs in invokeRegisteredAction before any action is allocated or the handler runs. Stored and returned schemas are now proto.Cloned.

I scanned the full PR diff for security and correctness issues. Mixed JWE+age recipient lists are already rejected upstream by crypto.ValidateEncryptionConfigs (pkg/crypto/crypto.go:82), so the guard cannot be bypassed that way. If the field is absent or set to explicit generic, the old code path runs unchanged. go.mod is unchanged; protoc-gen-validate was already a dependency.

Criteria triage:

• Silence: no. Every failure is a loud FailedPrecondition or a registration error.
• Durability: additive optional proto field only. No serialized pagination or sync state is touched.
• Uncontrolled dimensions: none. There is no new concurrency, and the cost is O(fields) per call.
• Consumer distance: yes. C1 reads the field, and an older C1 consumer is intentionally refused, loudly.

Verdict: low/medium risk, and no escalation is needed. The changes are additive; no exported signature or default changes. A patch release is acceptable.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes checks each return field independently. An action with one typed secret return plus other secret returns therefore registers successfully. It then fails on every call at secret_content_type.go:62-64 ("requires exactly one secret return"). That shape should be rejected at registration.
• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 (and plan.md). The ledger cites pre-squash commit hashes (528830db, 9a550250, 4483426d). These won't resolve after a squash merge. The process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes validates each return field independently, so a schema with a typed secret return plus additional secret returns registers successfully but fails on every invocation in validateTypedOutputContext. After the per-field loop, count secret returns; if any secret return has a non-"generic" content type and there is more than one secret return, return an error at registration. Add a registration test for that shape (global and resource).

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around line 3: The ledger references pre-squash commit hashes that will not resolve after merge. Move these process ledgers into the PR description, or drop the commit hashes.

[github-actions]

🟡 Suggestion (medium confidence): Each return field is validated on its own here. A schema with one typed secret return plus another secret return therefore registers successfully. It then fails on every invocation at validateTypedOutputContext ("requires exactly one secret return", line 62). Rejecting that shape here would show the mistake when the connector starts, not on the first call.

[github-actions]

Superseded — see the current review report for commit 490593b5d730

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

This PR adds an optional proto field, config.Field.secret_content_type (field 8, with PGV rules). Its number is not reused and generated pb, pb.validate and protoopaque output is included. The field is validated at registration: it is refused on arguments, public returns, and empty or malformed values. The declaration is captured into registeredActionHandler.secretReturnContentTypes when the action is registered. A typed output is refused with FailedPrecondition unless the single JWE recipient's AAD declares the matching c1/fk-credential-delivery v1 content type. That check runs in invokeRegisteredAction before any action is allocated or the handler runs. Stored and returned schemas are now proto.Cloned.

I scanned the full PR diff for security and correctness issues. Mixed JWE+age recipient lists are already rejected upstream by crypto.ValidateEncryptionConfigs (pkg/crypto/crypto.go:82), so the guard cannot be bypassed that way. If the field is absent or set to explicit generic, the old code path runs unchanged. go.mod is unchanged; protoc-gen-validate was already a dependency.

Criteria triage:

• Silence: no. Every failure is a loud FailedPrecondition or a registration error.
• Durability: additive optional proto field only. No serialized pagination or sync state is touched.
• Uncontrolled dimensions: none. There is no new concurrency, and the cost is O(fields) per call.
• Consumer distance: yes. C1 reads the field, and an older C1 consumer is intentionally refused, loudly.

Verdict: low/medium risk, and no escalation is needed. The changes are additive; no exported signature or default changes. A patch release is acceptable.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence): pkg/actions/secret_content_type.go:24-33. validateActionSecretContentTypes checks each return field independently. An action with one typed secret return plus other secret returns therefore registers successfully. It then fails on every call at secret_content_type.go:62-64 ("requires exactly one secret return"). That shape should be rejected at registration.
• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 (and plan.md). The ledger cites pre-squash commit hashes (528830db, 9a550250, 4483426d). These won't resolve after a squash merge. The process ledgers fit better in the PR description than in the SDK docs/ tree.

Prompt for AI agents

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

## Suggestions

In `pkg/actions/secret_content_type.go`:
- Around line 24-33: validateActionSecretContentTypes validates each return field independently, so a schema with a typed secret return plus additional secret returns registers successfully but fails on every invocation in validateTypedOutputContext. After the per-field loop, count secret returns; if any secret return has a non-"generic" content type and there is more than one secret return, return an error at registration. Add a registration test for that shape (global and resource).

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around line 3: The ledger references pre-squash commit hashes that will not resolve after merge. Move these process ledgers into the PR description, or drop the commit hashes.

---

Reviewed commit: c65a6c4b4c23

[github-actions]

Superseded — see the current review report for commit 020da7412067

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional field, config.Field.secret_content_type = 8, with PGV syntax rules and regenerated pb output. Action registration (global and resource, ordinary and with-secrets) rejects the field on arguments and on non-secret returns, and a typed declaration requires exactly one secret return. The handler snapshots each secret return's type. Before an action is allocated or the handler is dispatched, invokeRegisteredAction (pkg/actions/actions.go:927) requires that every JWE recipient carry a matching c1/fk-credential-delivery v1 context, and returns FailedPrecondition otherwise. Registered schemas are now cloned when stored and when read. I scanned the full diff for security and correctness and found no issues. The proto change is additive: a new field number, no renumbering, and generated code that matches the source. Generic and absent declarations keep their existing behavior.

The trusted criteria were applied as follows:

• Triage: Silence: no, because mistakes surface as registration errors or FailedPrecondition. Durability: yes, because this is a new wire field. Uncontrolled dimensions: yes, because the outcome depends on which caller version sends the request. Consumer distance: yes, because the C1 platform and connectors interpret the output. Verdict: HIGH (version-pair).
• Coverage: This falls in the multi-artifact / cross-version review-blind class. The diff contains a permutation table, TestTypedSecretOutputRefusesUnacknowledgedCallerBeforeHandler, which asserts zero handler calls and no allocation. It does not contain a two-artifact harness, so a pass-set review per docs/BUG_CATCHING.md §6 is still recommended before merge.
• Exported API: the only change is clone-on-read in ListActionSchemas/GetActionSchema. Callers that mutated returned schemas expecting the change to persist would lose that, but no supported contract relied on it.
• Defaults and dependencies: default paths are unchanged, there are no go.mod changes, and version.go is untouched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 and plan.md are a process ledger that cites pre-squash commit hashes (528830db, 9a550250, 4483426d). Those hashes won't resolve after a squash merge, and the ledger fits the PR description better than the SDK docs/ tree.

Resolved prior findings

• The config.proto comment that claimed secret_content_type is "not valid on configuration fields" is fixed. proto/c1/config/v1/config.proto:39-41 now says the field "has no configuration-field semantics; syntax validation still applies", which matches the registration-only enforcement in pkg/actions/secret_content_type.go:18-45.

Prompt for AI agents

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

## Suggestions

In `docs/verification/connector-secret-types/evidence.md`:
- Around line 3 (and plan.md generally): The ledger cites pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after a squash merge. Either move this process ledger into the PR description, or drop the commit hashes and refer to test names only.

[github-actions]

Superseded — see the current review report for commit 1a072f736f77

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds an optional field, config.Field.secret_content_type = 8, with PGV syntax rules and regenerated pb output. Action registration (global and resource, ordinary and with-secrets) rejects the field on arguments and on non-secret returns, and a typed declaration requires exactly one secret return. The handler snapshots each secret return's type. Before an action is allocated or the handler is dispatched, invokeRegisteredAction (pkg/actions/actions.go:927) requires that every JWE recipient carry a matching c1/fk-credential-delivery v1 context, and returns FailedPrecondition otherwise. Registered schemas are now cloned when stored and when read. I scanned the full diff for security and correctness and found no issues. The proto change is additive: a new field number, no renumbering, and generated code that matches the source. Generic and absent declarations keep their existing behavior.

The trusted criteria were applied as follows:

• Triage: Silence: no, because mistakes surface as registration errors or FailedPrecondition. Durability: yes, because this is a new wire field. Uncontrolled dimensions: yes, because the outcome depends on which caller version sends the request. Consumer distance: yes, because the C1 platform and connectors interpret the output. Verdict: HIGH (version-pair).
• Coverage: This falls in the multi-artifact / cross-version review-blind class. The diff contains a permutation table, TestTypedSecretOutputRefusesUnacknowledgedCallerBeforeHandler, which asserts zero handler calls and no allocation. It does not contain a two-artifact harness, so a pass-set review per docs/BUG_CATCHING.md §6 is still recommended before merge.
• Exported API: the only change is clone-on-read in ListActionSchemas/GetActionSchema. Callers that mutated returned schemas expecting the change to persist would lose that, but no supported contract relied on it.
• Defaults and dependencies: default paths are unchanged, there are no go.mod changes, and version.go is untouched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (low confidence): docs/verification/connector-secret-types/evidence.md:3 and plan.md are a process ledger that cites pre-squash commit hashes (528830db, 9a550250, 4483426d). Those hashes won't resolve after a squash merge, and the ledger fits the PR description better than the SDK docs/ tree.

Resolved prior findings

• The config.proto comment that claimed secret_content_type is "not valid on configuration fields" is fixed. proto/c1/config/v1/config.proto:39-41 now says the field "has no configuration-field semantics; syntax validation still applies", which matches the registration-only enforcement in pkg/actions/secret_content_type.go:18-45.

Prompt for AI agents

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

## Suggestions

In `docs/verification/connector-secret-types/evidence.md`:
- Around line 3 (and plan.md generally): The ledger cites pre-squash commit hashes (528830db, 9a550250, 4483426d) that will not resolve after a squash merge. Either move this process ledger into the PR description, or drop the commit hashes and refer to test names only.

---

Reviewed commit: 020da7412067

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 1a072f736f77

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The new commits change how a field declares its type:

• Proto: Field.secret_content_type (optional, field 8) is replaced by content_type (a MIME string, field 8) and repeated tags (field 9).
• Logical type: it now comes from a single multipass-type-<type> tag, resolved by the new crypto.ResolveFieldOutput.
• Context check: the typed-context matcher moved to pkg/crypto.
• Transport: the Lambda transport gains a JSON pre-scan that rejects secret_content_type / secretContentType on any c1.config.v1.Field.

Scope and coverage:

• The full PR diff was scanned for security and correctness, including the two generated pb.go files that the incremental artifact dropped. Those files agree with the proto: fields 8 and 9 are tagged correctly and no SecretContentType symbols remain.
• Reusing field 8 is not a wire break against main, because the base Field has no field 8 (is_secret = 7, then the oneof at 100).

Criteria applied:

• Triage: Silence: no, because failures surface as registration or transport errors. Durability: yes, because these are new proto fields. Uncontrolled dimensions: yes, because the outcome depends on which caller or connector version is on each side. Consumer distance: yes, because the C1 platform and connectors read these fields. Verdict: HIGH (version-pair). The permutation tests and TestLegacyBinaryFieldEightRejectedAtRegistration are present, but there is no two-artifact cross-version harness. The §6 pass-set review is still recommended before merge.
• Exported API: the exported API change is additive (ContentType/Tags getters, crypto.ResolveFieldOutput, ValidSecretContentType, TypedOutputContextMatches).
• Defaults and dependencies: there are no go.mod changes, and version.go is untouched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New pkg/lambda/grpc/transport.go:51 (medium confidence): rejectLegacyFieldJSON runs on every legacy-JSON payload before protojson.
  • It recursively re-decodes each nested message object into a map, which adds cost on the default path for all Lambda traffic.
  • It contradicts the "fast path is a plain protojson unmarshal" comment at transport.go:42-45.
  • It guards a field that main never shipped, so the rejection path can only fire for prototype-branch builds.
• Prior — still present (low confidence; the stale ledger docs are worse after this push):
  • docs/verification/connector-secret-types/evidence.md and plan.md still describe config.Field.secret_content_type=8 and HasSecretContentType. Neither exists after this push. They also cite pre-squash commit hashes.
  • The new field-metadata-transition.md:8 says "Native request_schema and descriptor secret_content_type remain readable". Neither surface exists in this PR.
  • A reader following these docs would look for APIs that aren't there. Update or remove them, or move the ledger into the PR description.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 51: rejectLegacyFieldJSON runs on every legacy-JSON payload before the protojson fast path. It recursively re-decodes nested objects, and it guards Field.secret_content_type, which never shipped on main. Remove the pre-scan, or run it only after protojson fails. Then update the doc comment around lines 42-45 so it still describes the actual fast path. Adjust TestLegacyFieldJSONIsRefused and TestLegacyFieldJSONAnyGuard in wire_test.go to match.

In `docs/verification/connector-secret-types/evidence.md`, `plan.md`, `field-metadata-transition.md`:
- evidence.md and plan.md describe config.Field.secret_content_type=8 and HasSecretContentType, which were replaced by content_type, tags and the multipass-type- tag. They also cite pre-squash commit hashes.
- field-metadata-transition.md line 8 mentions request_schema and descriptor secret_content_type, which do not exist in this PR.
- Update these docs to match the current API, or remove the ledger from docs/ and keep it in the PR description.

[github-actions]

🟡 Suggestion (medium confidence): This pre-scan now runs on every legacy-JSON request and response, before the protojson fast path. Each nested message object gets re-decoded into a map, so the cost grows with payload size × nesting depth, and large List* responses pay it on the default path. It contradicts the doc comment above ("The fast path is a plain protojson unmarshal").

It also guards a field that main never shipped: field 8 doesn't exist at the base, and DiscardUnknown would drop it anyway. Consider removing the scan, or running it only after protojson fails, and updating the comment to match.

[github-actions]

Superseded — see the current review report for commit 5bae0e6c8026

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The new commits change how a field declares its type:

• Proto: Field.secret_content_type (optional, field 8) is replaced by content_type (a MIME string, field 8) and repeated tags (field 9).
• Logical type: it now comes from a single multipass-type-<type> tag, resolved by the new crypto.ResolveFieldOutput.
• Context check: the typed-context matcher moved to pkg/crypto.
• Transport: the Lambda transport gains a JSON pre-scan that rejects secret_content_type / secretContentType on any c1.config.v1.Field.

Scope and coverage:

• The full PR diff was scanned for security and correctness, including the two generated pb.go files that the incremental artifact dropped. Those files agree with the proto: fields 8 and 9 are tagged correctly and no SecretContentType symbols remain.
• Reusing field 8 is not a wire break against main, because the base Field has no field 8 (is_secret = 7, then the oneof at 100).

Criteria applied:

• Triage: Silence: no, because failures surface as registration or transport errors. Durability: yes, because these are new proto fields. Uncontrolled dimensions: yes, because the outcome depends on which caller or connector version is on each side. Consumer distance: yes, because the C1 platform and connectors read these fields. Verdict: HIGH (version-pair). The permutation tests and TestLegacyBinaryFieldEightRejectedAtRegistration are present, but there is no two-artifact cross-version harness. The §6 pass-set review is still recommended before merge.
• Exported API: the exported API change is additive (ContentType/Tags getters, crypto.ResolveFieldOutput, ValidSecretContentType, TypedOutputContextMatches).
• Defaults and dependencies: there are no go.mod changes, and version.go is untouched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New pkg/lambda/grpc/transport.go:51 (medium confidence): rejectLegacyFieldJSON runs on every legacy-JSON payload before protojson.
  • It recursively re-decodes each nested message object into a map, which adds cost on the default path for all Lambda traffic.
  • It contradicts the "fast path is a plain protojson unmarshal" comment at transport.go:42-45.
  • It guards a field that main never shipped, so the rejection path can only fire for prototype-branch builds.
• Prior — still present (low confidence; the stale ledger docs are worse after this push):
  • docs/verification/connector-secret-types/evidence.md and plan.md still describe config.Field.secret_content_type=8 and HasSecretContentType. Neither exists after this push. They also cite pre-squash commit hashes.
  • The new field-metadata-transition.md:8 says "Native request_schema and descriptor secret_content_type remain readable". Neither surface exists in this PR.
  • A reader following these docs would look for APIs that aren't there. Update or remove them, or move the ledger into the PR description.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 51: rejectLegacyFieldJSON runs on every legacy-JSON payload before the protojson fast path. It recursively re-decodes nested objects, and it guards Field.secret_content_type, which never shipped on main. Remove the pre-scan, or run it only after protojson fails. Then update the doc comment around lines 42-45 so it still describes the actual fast path. Adjust TestLegacyFieldJSONIsRefused and TestLegacyFieldJSONAnyGuard in wire_test.go to match.

In `docs/verification/connector-secret-types/evidence.md`, `plan.md`, `field-metadata-transition.md`:
- evidence.md and plan.md describe config.Field.secret_content_type=8 and HasSecretContentType, which were replaced by content_type, tags and the multipass-type- tag. They also cite pre-squash commit hashes.
- field-metadata-transition.md line 8 mentions request_schema and descriptor secret_content_type, which do not exist in this PR.
- Update these docs to match the current API, or remove the ledger from docs/ and keep it in the PR description.

---

Reviewed commit: 1a072f736f77

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 5bae0e6c8026

General PR Review: Declare typed secret action outputs before producer execution

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 98b0cb1f9b13.
Review mode: incremental since 1a072f7
View review run

Review Summary

The new commit (5bae0e6) only reformats the Field.tags validate-rule message literal in proto/c1/config/v1/config.proto, spreading it across several lines. The constraints are unchanged (min_len 1, max_len 143, pattern, unique, max_items 16), so the descriptor semantics and the checked-in pb.go/pb.validate.go output are unaffected. No regeneration is needed. I rescanned the full PR diff (14 files) for security and correctness and found nothing new.

I applied the repo-local criteria. Proto wire compatibility is unchanged by this push: field 8 content_type and field 9 tags are still tagged as before. The earlier triage still holds: HIGH because of version-pair dependence, with permutation tests present but no two-artifact cross-version harness, so the §6 pass-set review is still recommended before merge. There are no go.mod or version.go changes. The incremental artifact was complete, with nothing dropped or truncated.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/lambda/grpc/transport.go:51: rejectLegacyFieldJSON runs on every non-wire-frame JSON payload before protojson.
  • This contradicts the doc comment at transport.go:42-45, which says "the fast path is a plain protojson unmarshal".
  • It guards a field (secret_content_type) that main never shipped.
• Prior — still present docs/verification/connector-secret-types/evidence.md:7 and plan.md:40: both still describe config.Field.secret_content_type=8 and HasSecretContentType, and neither exists in the current code.
• Prior — still present docs/verification/connector-secret-types/field-metadata-transition.md:8: it says native request_schema and descriptor secret_content_type "remain readable", but neither surface exists in this PR.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 42-51: rejectLegacyFieldJSON now runs on every legacy JSON payload before the protojson fast path, contradicting the doc comment that says the fast path is a plain protojson unmarshal. Either move the rejection so it only runs where needed (or after unmarshal for Field-bearing messages), or update the comment to describe the pre-scan and its cost. Consider whether guarding a field that main never shipped is needed at all.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around evidence.md line 7 and plan.md line 40: Replace references to `config.Field.secret_content_type=8` and `HasSecretContentType` with the current `content_type` (field 8) / `tags` (field 9) design and `crypto.ResolveFieldOutput`, and drop pre-squash commit hashes.

In `docs/verification/connector-secret-types/field-metadata-transition.md`:
- Around line 8: Remove or correct the claim that native `request_schema` and descriptor `secret_content_type` remain readable; neither surface exists in this PR.

[github-actions]

Superseded — see the current review report for commit 69b9808260b0

General PR Review: Declare typed secret action outputs before producer execution

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 98b0cb1f9b13.
Review mode: incremental since 1a072f7
View review run

Review Summary

The new commit (5bae0e6) only reformats the Field.tags validate-rule message literal in proto/c1/config/v1/config.proto, spreading it across several lines. The constraints are unchanged (min_len 1, max_len 143, pattern, unique, max_items 16), so the descriptor semantics and the checked-in pb.go/pb.validate.go output are unaffected. No regeneration is needed. I rescanned the full PR diff (14 files) for security and correctness and found nothing new.

I applied the repo-local criteria. Proto wire compatibility is unchanged by this push: field 8 content_type and field 9 tags are still tagged as before. The earlier triage still holds: HIGH because of version-pair dependence, with permutation tests present but no two-artifact cross-version harness, so the §6 pass-set review is still recommended before merge. There are no go.mod or version.go changes. The incremental artifact was complete, with nothing dropped or truncated.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/lambda/grpc/transport.go:51: rejectLegacyFieldJSON runs on every non-wire-frame JSON payload before protojson.
  • This contradicts the doc comment at transport.go:42-45, which says "the fast path is a plain protojson unmarshal".
  • It guards a field (secret_content_type) that main never shipped.
• Prior — still present docs/verification/connector-secret-types/evidence.md:7 and plan.md:40: both still describe config.Field.secret_content_type=8 and HasSecretContentType, and neither exists in the current code.
• Prior — still present docs/verification/connector-secret-types/field-metadata-transition.md:8: it says native request_schema and descriptor secret_content_type "remain readable", but neither surface exists in this PR.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 42-51: rejectLegacyFieldJSON now runs on every legacy JSON payload before the protojson fast path, contradicting the doc comment that says the fast path is a plain protojson unmarshal. Either move the rejection so it only runs where needed (or after unmarshal for Field-bearing messages), or update the comment to describe the pre-scan and its cost. Consider whether guarding a field that main never shipped is needed at all.

In `docs/verification/connector-secret-types/evidence.md` and `plan.md`:
- Around evidence.md line 7 and plan.md line 40: Replace references to `config.Field.secret_content_type=8` and `HasSecretContentType` with the current `content_type` (field 8) / `tags` (field 9) design and `crypto.ResolveFieldOutput`, and drop pre-squash commit hashes.

In `docs/verification/connector-secret-types/field-metadata-transition.md`:
- Around line 8: Remove or correct the claim that native `request_schema` and descriptor `secret_content_type` remain readable; neither surface exists in this PR.

---

Reviewed commit: 5bae0e6c8026

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 69b9808260b0

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds two fields to config.Field: content_type (8, a MIME type) and tags (9). The proto change and regenerated pb output (.pb.go, protoopaque, validate) match, and protoc-gen-validate is already in go.mod.

crypto.ResolveFieldOutput reads a logical type from a single multipass-type- tag. For secret actions, pkg/actions then:

• validates where that type is declared and how many secret returns there are, at registration on all four Register paths;
• snapshots the declared types per handler;
• checks before allocation or handler dispatch that every JWE recipient's AAD (purpose/version/content_type) agrees with the declared type.

Schemas are now cloned on registration, in ListActionSchemas, and in GetActionSchema. The lambda transport rejects legacy JSON that carries a secret_content_type key on a Field. Undeclared and generic outputs keep their old behavior.

I scanned the full diff for security and correctness and found no blocking issues. The AAD parser rejects duplicate and escaped-duplicate keys, trailing data, and oversize input. The guard runs after ValidateEncryptionConfigs and before GetNewAction.

Criteria:

• Triage:
  • Silence: low. A mismatch fails closed with FailedPrecondition.
  • Durability: yes. These are new proto wire fields.
  • Uncontrolled dimensions: version-pair, but bounded. Old callers get a refusal, not a silent reinterpretation.
  • Consumer distance: C1.
• Verdict: HIGH on the wire-contract axis, mitigated because the change is additive. The PR includes the permutation-table tests the criteria ask for: global/resource × context variants, cardinality, snapshot mutation, and a mixed-version refusal. Before release, confirm field numbers 8 and 9 against the C1-side config.proto; I could not check that here.
• Proto rules: fields are additive, nothing is renumbered, and generated code is regenerated.
• Defaults: unchanged when no type is declared.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/lambda/grpc/transport.go:51: rejectLegacyFieldJSON runs on every legacy (non-wire-frame) request and response.
  • It does a full encoding/json decode and re-decodes every nested subtree, so cost scales with payload size × depth.
  • That contradicts the comment at transport.go:41-44 ("the fast path is a plain protojson unmarshal").
  • It also guards secret_content_type, a field main never shipped.
• Prior — still present docs/verification/connector-secret-types/evidence.md:7 and plan.md:40: both still describe HasSecretContentType and config.Field.secret_content_type=8 and cite pre-squash commit hashes. None of these exist in the current code.
• Prior — still present docs/verification/connector-secret-types/field-metadata-transition.md:8: claims native request_schema and descriptor secret_content_type "remain readable". Neither surface exists in this PR.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 51 (and the rejectLegacyFieldJSON function near line 362): the guard re-parses every legacy transport payload with encoding/json, and recursively re-parses nested subtrees, before protojson runs. Either remove the guard, since `secret_content_type` was never shipped on main and protojson with DiscardUnknown already ignores it, or limit it to payloads that can actually carry a config.Field. Then update the doc comment at lines 41-44 so it matches the real fast path.

In `docs/verification/connector-secret-types/evidence.md`:
- Around lines 3-7: replace the references to `HasSecretContentType`, `config.Field.secret_content_type=8` and the pre-squash commit hashes with the current design: `content_type` (field 8, MIME) plus a `multipass-type-` entry in `tags` (field 9), resolved by `crypto.ResolveFieldOutput`.

In `docs/verification/connector-secret-types/plan.md`:
- Around line 40 (change order 1): make the same correction. It still names `config.Field.secret_content_type=8`.

In `docs/verification/connector-secret-types/field-metadata-transition.md`:
- Around line 8: remove or correct the claim that native `request_schema` and descriptor `secret_content_type` remain readable. Neither surface exists in this PR; move the claim to the PR that introduces them.

[github-actions]

🟡 Suggestion (medium confidence): rejectLegacyFieldJSON now runs on every legacy (non-wire-frame) payload before protojson. It decodes the document with encoding/json and re-decodes each nested subtree, so cost scales with payload size × depth. That contradicts the comment at lines 41-44 ("the fast path is a plain protojson unmarshal"). It also guards secret_content_type, a field main never shipped. Consider dropping the guard, or gating it, and updating the comment.

[github-actions]

🟡 Suggestion: This ledger (and plan.md change order 1) still describes HasSecretContentType and config.Field.secret_content_type=8, and it cites pre-squash commit hashes. None of those exist in this PR. The current design is content_type (8) plus a multipass-type- entry in tags (9), resolved by crypto.ResolveFieldOutput. Please update these references or remove them.

[github-actions]

🟡 Suggestion: This says native request_schema and descriptor secret_content_type "remain readable", and that new and legacy declarations must agree. Neither surface exists in this PR. A reader would expect a compatibility path that doesn't exist, so please remove the claim or move it to the PR that adds those surfaces (#1195).

[github-actions]

General PR Review: Declare typed secret action outputs before producer execution

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

Review Summary

The PR adds two fields to config.Field: content_type (8, a MIME type) and tags (9). The proto change and regenerated pb output (.pb.go, protoopaque, validate) match, and protoc-gen-validate is already in go.mod.

crypto.ResolveFieldOutput reads a logical type from a single multipass-type- tag. For secret actions, pkg/actions then:

• validates where that type is declared and how many secret returns there are, at registration on all four Register paths;
• snapshots the declared types per handler;
• checks before allocation or handler dispatch that every JWE recipient's AAD (purpose/version/content_type) agrees with the declared type.

Schemas are now cloned on registration, in ListActionSchemas, and in GetActionSchema. The lambda transport rejects legacy JSON that carries a secret_content_type key on a Field. Undeclared and generic outputs keep their old behavior.

I scanned the full diff for security and correctness and found no blocking issues. The AAD parser rejects duplicate and escaped-duplicate keys, trailing data, and oversize input. The guard runs after ValidateEncryptionConfigs and before GetNewAction.

Criteria:

• Triage:
  • Silence: low. A mismatch fails closed with FailedPrecondition.
  • Durability: yes. These are new proto wire fields.
  • Uncontrolled dimensions: version-pair, but bounded. Old callers get a refusal, not a silent reinterpretation.
  • Consumer distance: C1.
• Verdict: HIGH on the wire-contract axis, mitigated because the change is additive. The PR includes the permutation-table tests the criteria ask for: global/resource × context variants, cardinality, snapshot mutation, and a mixed-version refusal. Before release, confirm field numbers 8 and 9 against the C1-side config.proto; I could not check that here.
• Proto rules: fields are additive, nothing is renumbered, and generated code is regenerated.
• Defaults: unchanged when no type is declared.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• Prior — still present (medium confidence) pkg/lambda/grpc/transport.go:51: rejectLegacyFieldJSON runs on every legacy (non-wire-frame) request and response.
  • It does a full encoding/json decode and re-decodes every nested subtree, so cost scales with payload size × depth.
  • That contradicts the comment at transport.go:41-44 ("the fast path is a plain protojson unmarshal").
  • It also guards secret_content_type, a field main never shipped.
• Prior — still present docs/verification/connector-secret-types/evidence.md:7 and plan.md:40: both still describe HasSecretContentType and config.Field.secret_content_type=8 and cite pre-squash commit hashes. None of these exist in the current code.
• Prior — still present docs/verification/connector-secret-types/field-metadata-transition.md:8: claims native request_schema and descriptor secret_content_type "remain readable". Neither surface exists in this PR.

Prompt for AI agents

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

## Suggestions

In `pkg/lambda/grpc/transport.go`:
- Around line 51 (and the rejectLegacyFieldJSON function near line 362): the guard re-parses every legacy transport payload with encoding/json, and recursively re-parses nested subtrees, before protojson runs. Either remove the guard, since `secret_content_type` was never shipped on main and protojson with DiscardUnknown already ignores it, or limit it to payloads that can actually carry a config.Field. Then update the doc comment at lines 41-44 so it matches the real fast path.

In `docs/verification/connector-secret-types/evidence.md`:
- Around lines 3-7: replace the references to `HasSecretContentType`, `config.Field.secret_content_type=8` and the pre-squash commit hashes with the current design: `content_type` (field 8, MIME) plus a `multipass-type-` entry in `tags` (field 9), resolved by `crypto.ResolveFieldOutput`.

In `docs/verification/connector-secret-types/plan.md`:
- Around line 40 (change order 1): make the same correction. It still names `config.Field.secret_content_type=8`.

In `docs/verification/connector-secret-types/field-metadata-transition.md`:
- Around line 8: remove or correct the claim that native `request_schema` and descriptor `secret_content_type` remain readable. Neither surface exists in this PR; move the claim to the PR that introduces them.

---

Reviewed commit: 69b9808260b0

[github-actions]

No blocking issues found — see the full review report