feat: Add NICo Core gRPC passthrough for admin operations#2360
feat: Add NICo Core gRPC passthrough for admin operations#2360kfelternv wants to merge 2 commits into
Conversation
Signed-off-by: Kyle Felter <kfelter@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-10 17:50:35 UTC | Commit: 4971eab |
|
I don't see a description (can you write one)? Also, I think it's probably not necessary to put 'nico' in the name of any of this and can just be generic names like backend, core, or whatever. |
|
... also I'm sure there's existing REST -> gRPC proxies that already exists? I haven't read the code in depth to grok it. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
rest-api/flow/internal/nicoapi/passthrough.go (1)
162-163: ⚡ Quick winPrefer range-over-integer instead of a C-style counting loop.
This loop can use Go’s range-over-integer form for consistency with repo standards.
Suggested change
- for i := 0; i < methods.Len(); i++ { + for i := range methods.Len() { md := methods.Get(i)As per coding guidelines,
rest-api/**/*.goshould prefer range-based iteration over C-style loops when range expresses the loop.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/nicoapi/passthrough.go` around lines 162 - 163, Replace the C-style counting loop with a range-over-integer form: change the for loop that uses methods.Len() (currently "for i := 0; i < methods.Len(); i++ { md := methods.Get(i) }") to a range-based loop such as "for i := range make([]struct{}, methods.Len()) { md := methods.Get(i) }" so iteration uses Go's range form while still obtaining md via methods.Get(i).Source: Coding guidelines
rest-api/openapi/spec.yaml (2)
357-358: 💤 Low valueClarify mutation classification with explicit read-only prefixes.
The description currently phrases the classification as a negative ("anything other than Find/Get/List/Search/Lookup/Identify/Determine/Version/Echo"), which requires the reader to invert the logic. Consider rephrasing to state the positive condition directly: "Methods are classified as mutations unless their name begins with one of the following prefixes: Find, Get, List, Search, Lookup, Identify, Determine, Version, or Echo."
This aligns better with the implementation logic in
nicopassthrough.IsMutationand makes the rule easier to validate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/openapi/spec.yaml` around lines 357 - 358, Rephrase the OpenAPI description to state the positive rule: replace the negative phrasing with a sentence like "Methods are classified as mutations unless their name begins with one of the following prefixes: Find, Get, List, Search, Lookup, Identify, Determine, Version, or Echo; mutations additionally require `allowMutation: true`." Update the text near the existing line in spec.yaml so it matches the behavior in nicopassthrough.IsMutation and lists those prefixes explicitly for clarity.
379-387: ⚡ Quick winDocument 400 and 401 status codes for completeness.
While the GET endpoint has less input validation than POST, it still goes through the same authorization and workflow execution paths. Document
400(for malformed siteId or other request issues) and401(for missing/invalid auth token) to match typical REST API contracts and improve discoverability for client developers.📝 Suggested addition
responses: '200': description: OK content: application/json: schema: $ref: '`#/components/schemas/NICoCoreMethodsResponse`' + '400': + $ref: '`#/components/responses/BadRequestError`' + '401': + $ref: '`#/components/responses/UnauthorizedError`' '403': $ref: '`#/components/responses/ForbiddenError`'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/openapi/spec.yaml` around lines 379 - 387, Add explicit '400' and '401' responses to the same responses block that currently contains '200' -> $ref: '`#/components/schemas/NICoCoreMethodsResponse`' and '403' -> '`#/components/responses/ForbiddenError`'; specifically add a '400' entry referencing a BadRequest response (e.g. '`#/components/responses/BadRequestError`') for malformed siteId/params and a '401' entry referencing an Unauthorized response (e.g. '`#/components/responses/UnauthorizedError`') for missing/invalid auth token — if those components don't exist, create them under components/responses with appropriate description and JSON content schema.rest-api/flow/internal/nicoapi/passthrough_test.go (2)
114-122: ⚡ Quick winStrengthen method-catalog assertions so missing baseline methods fail the test.
The
if v, ok := ...; okpattern silently passes ifFindMachineIds,Version, orCreateVpcare missing. These should be required entries for this contract test.Suggested test hardening
- if v, ok := byName["FindMachineIds"]; ok { - assert.False(t, v.Mutation, "FindMachineIds should be classified read-only") - } - if v, ok := byName["Version"]; ok { - assert.False(t, v.Mutation, "Version should be classified read-only") - } - if v, ok := byName["CreateVpc"]; ok { - assert.True(t, v.Mutation, "CreateVpc should be classified as a mutation") - } + require.Contains(t, byName, "FindMachineIds") + assert.False(t, byName["FindMachineIds"].Mutation, "FindMachineIds should be classified read-only") + + require.Contains(t, byName, "Version") + assert.False(t, byName["Version"].Mutation, "Version should be classified read-only") + + require.Contains(t, byName, "CreateVpc") + assert.True(t, byName["CreateVpc"].Mutation, "CreateVpc should be classified as a mutation")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/nicoapi/passthrough_test.go` around lines 114 - 122, The test currently uses optional checks like `if v, ok := byName["FindMachineIds"]; ok { ... }` which silently passes if an entry is missing; update the assertions to require the entries exist and then assert their mutation flags. For each symbol `FindMachineIds`, `Version`, and `CreateVpc` in the `byName` map, first assert existence (e.g., assert.True(ok, "expected method <Name> to be present")) and only then assert `v.Mutation` is false for `FindMachineIds` and `Version`, and true for `CreateVpc` so missing baseline methods cause the test to fail.
41-47: ⚡ Quick winUse range-over-integer for descriptor field iteration.
Line 41 currently uses a C-style counting loop where
rangeover integer is supported in this repo’s Go target. Switching tofor i := range fields.Len()keeps the loop idiomatic and reduces index bookkeeping.As per coding guidelines, “Prefer range-based iteration over C-style
forloops … usefor i := range nfor counting loops.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/nicoapi/passthrough_test.go` around lines 41 - 47, Replace the C-style index loop in the test (the `for i := 0; i < fields.Len(); i++ {` block) with a range-based index loop so you iterate indices idiomatically (use `for i := range ...`), keeping the body unchanged (calling `fields.Get(i)`, checking `fd.Kind()`/`fd.Cardinality()`, and calling `pm.Set(fd, protoreflect.ValueOfString(f.setValue))`). This preserves the existing logic with `fields`, `fd`, `pm.Set`, and `f.setValue` while following the repository's preferred range-over-integer style.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rest-api/flow/cmd/serve.go`:
- Around line 239-243: The Passthrough gRPC client created by
nicoapi.NewPassthrough in doServe is stored to temporalManagerConf.CoreInvoker
but never closed; update the success branch where passthrough is assigned to
call passthrough.Close() on shutdown (e.g., defer passthrough.Close() or
register a cleanup) so the underlying gRPC connection is closed when doServe
exits; specifically add the deferred close in the block that sets
temporalManagerConf.CoreInvoker (referencing NewPassthrough, passthrough,
temporalManagerConf.CoreInvoker, and doServe).
In `@rest-api/flow/internal/nicoapi/passthrough.go`:
- Around line 137-139: Change the JSON decoding in invokeJSON to be strict (stop
discarding unknown fields) so unknown/typoed fields cause an error instead of
silently producing zero values: replace the current
protojson.UnmarshalOptions{DiscardUnknown: true} usage in invokeJSON with
options that do not discard unknown fields (i.e., DiscardUnknown: false or omit
the option) so Unmarshal fails on unknown fields and return that error; also
update the iteration in listMethods to use Go’s idiomatic range-over-integer
form (use for i := range ... or for i := 0; i < methods.Len(); i++ -> for i :=
range make([]struct{}, methods.Len()) style replacement) to match the repo’s
loop style.
In `@rest-api/openapi/spec.yaml`:
- Around line 342-350: The OpenAPI spec for the NICoCore passthrough endpoint is
missing the documented 400 response even though nicocorepassthrough.go returns
400 for invalid request bodies and missing "method"; update the spec under the
endpoint responses to include a '400' response (e.g., reference an existing
BadRequestError response component or add a new
components/responses/BadRequestError and reference it) so clients see the
client-error condition and match the handler behavior.
- Around line 12844-12855: The OpenAPI schema NICoCorePassthroughResponse must
declare its always-populated fields as required; update the
NICoCorePassthroughResponse object to include a required array listing "method"
and "response" so the spec matches the handler that always sets those fields
(see the NICo Core passthrough handler that populates method and response).
- Around line 12856-12867: The OpenAPI schema NICoCoreMethodsResponse is missing
a required constraint for fields that are always set; update the
NICoCoreMethodsResponse schema to include a required array with "service" and
"methods" so both properties are marked mandatory (i.e., add required:
["service","methods"])—this matches the handler that always populates those
fields (see NICoCoreMethodsResponse schema and the nicocore passthrough handler
that constructs the response).
- Around line 12868-12884: Update the NICoCoreMethod schema to mark all six
properties as required so client SDKs reflect the actual contract: add a
required array containing "method", "fullMethod", "inputType", "outputType",
"mutation", and "deprecated" within the NICoCoreMethod object definition (the
schema named NICoCoreMethod) so those fields are validated as present.
---
Nitpick comments:
In `@rest-api/flow/internal/nicoapi/passthrough_test.go`:
- Around line 114-122: The test currently uses optional checks like `if v, ok :=
byName["FindMachineIds"]; ok { ... }` which silently passes if an entry is
missing; update the assertions to require the entries exist and then assert
their mutation flags. For each symbol `FindMachineIds`, `Version`, and
`CreateVpc` in the `byName` map, first assert existence (e.g., assert.True(ok,
"expected method <Name> to be present")) and only then assert `v.Mutation` is
false for `FindMachineIds` and `Version`, and true for `CreateVpc` so missing
baseline methods cause the test to fail.
- Around line 41-47: Replace the C-style index loop in the test (the `for i :=
0; i < fields.Len(); i++ {` block) with a range-based index loop so you iterate
indices idiomatically (use `for i := range ...`), keeping the body unchanged
(calling `fields.Get(i)`, checking `fd.Kind()`/`fd.Cardinality()`, and calling
`pm.Set(fd, protoreflect.ValueOfString(f.setValue))`). This preserves the
existing logic with `fields`, `fd`, `pm.Set`, and `f.setValue` while following
the repository's preferred range-over-integer style.
In `@rest-api/flow/internal/nicoapi/passthrough.go`:
- Around line 162-163: Replace the C-style counting loop with a
range-over-integer form: change the for loop that uses methods.Len() (currently
"for i := 0; i < methods.Len(); i++ { md := methods.Get(i) }") to a range-based
loop such as "for i := range make([]struct{}, methods.Len()) { md :=
methods.Get(i) }" so iteration uses Go's range form while still obtaining md via
methods.Get(i).
In `@rest-api/openapi/spec.yaml`:
- Around line 357-358: Rephrase the OpenAPI description to state the positive
rule: replace the negative phrasing with a sentence like "Methods are classified
as mutations unless their name begins with one of the following prefixes: Find,
Get, List, Search, Lookup, Identify, Determine, Version, or Echo; mutations
additionally require `allowMutation: true`." Update the text near the existing
line in spec.yaml so it matches the behavior in nicopassthrough.IsMutation and
lists those prefixes explicitly for clarity.
- Around line 379-387: Add explicit '400' and '401' responses to the same
responses block that currently contains '200' -> $ref:
'`#/components/schemas/NICoCoreMethodsResponse`' and '403' ->
'`#/components/responses/ForbiddenError`'; specifically add a '400' entry
referencing a BadRequest response (e.g.
'`#/components/responses/BadRequestError`') for malformed siteId/params and a
'401' entry referencing an Unauthorized response (e.g.
'`#/components/responses/UnauthorizedError`') for missing/invalid auth token — if
those components don't exist, create them under components/responses with
appropriate description and JSON content schema.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f8590879-8b29-4b9c-b981-5c0353298cd0
📒 Files selected for processing (16)
rest-api/api/pkg/api/handler/nicocorepassthrough.gorest-api/api/pkg/api/handler/nicocorepassthrough_test.gorest-api/api/pkg/api/model/nicocorepassthrough.gorest-api/api/pkg/api/routes.gorest-api/common/pkg/nicopassthrough/nicopassthrough.gorest-api/common/pkg/nicopassthrough/nicopassthrough_test.gorest-api/flow/cmd/serve.gorest-api/flow/internal/nicoapi/passthrough.gorest-api/flow/internal/nicoapi/passthrough_test.gorest-api/flow/internal/task/executor/temporalworkflow/activity/activity_test.gorest-api/flow/internal/task/executor/temporalworkflow/activity/passthrough.gorest-api/flow/internal/task/executor/temporalworkflow/activity/registry.gorest-api/flow/internal/task/executor/temporalworkflow/activity/registry_test.gorest-api/flow/internal/task/executor/temporalworkflow/manager/manager.gorest-api/flow/internal/task/executor/temporalworkflow/workflow/passthrough.gorest-api/openapi/spec.yaml
| if passthrough, err := nicoapi.NewPassthrough(time.Minute); err != nil { | ||
| log.Warn().Err(err).Msg("NICo Core gRPC passthrough disabled") | ||
| } else { | ||
| temporalManagerConf.CoreInvoker = passthrough | ||
| } |
There was a problem hiding this comment.
Close the passthrough client on shutdown to avoid leaking the gRPC connection.
On successful initialization, the new Passthrough connection is stored but never explicitly closed in doServe. Add a deferred close in this success path.
Suggested change
if passthrough, err := nicoapi.NewPassthrough(time.Minute); err != nil {
log.Warn().Err(err).Msg("NICo Core gRPC passthrough disabled")
} else {
temporalManagerConf.CoreInvoker = passthrough
+ defer passthrough.Close()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if passthrough, err := nicoapi.NewPassthrough(time.Minute); err != nil { | |
| log.Warn().Err(err).Msg("NICo Core gRPC passthrough disabled") | |
| } else { | |
| temporalManagerConf.CoreInvoker = passthrough | |
| } | |
| if passthrough, err := nicoapi.NewPassthrough(time.Minute); err != nil { | |
| log.Warn().Err(err).Msg("NICo Core gRPC passthrough disabled") | |
| } else { | |
| temporalManagerConf.CoreInvoker = passthrough | |
| defer passthrough.Close() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/flow/cmd/serve.go` around lines 239 - 243, The Passthrough gRPC
client created by nicoapi.NewPassthrough in doServe is stored to
temporalManagerConf.CoreInvoker but never closed; update the success branch
where passthrough is assigned to call passthrough.Close() on shutdown (e.g.,
defer passthrough.Close() or register a cleanup) so the underlying gRPC
connection is closed when doServe exits; specifically add the deferred close in
the block that sets temporalManagerConf.CoreInvoker (referencing NewPassthrough,
passthrough, temporalManagerConf.CoreInvoker, and doServe).
| if err := (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal(reqJSON, in); err != nil { | ||
| return nil, fmt.Errorf("decode request for %q: %w", md.Name(), err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rest-api/flow/internal/nicoapi/passthrough.go"
echo "== File =="
ls -l "$FILE"
echo
echo "== Lines 120-190 =="
nl -ba "$FILE" | sed -n '110,210p'
echo
echo "== Search for DiscardUnknown / UnmarshalOptions usages =="
rg -n "DiscardUnknown|UnmarshalOptions|protojson\.Unmarshal" "$FILE"
echo
echo "== Search for C-style loops in this file =="
rg -n "for\s+[a-zA-Z_]+\s*:=\s*0;|for\s+[a-zA-Z_]+\s*:=.*;.*;.*\-\-|for\s+[a-zA-Z_]+\s*:=.*;.*<.*;|for\s+[a-zA-Z_]+\s*:=.*;.*<=.*;|for\s*.*;.*;.*" "$FILE" || trueRepository: NVIDIA/infra-controller
Length of output: 256
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="rest-api/flow/internal/nicoapi/passthrough.go"
echo "== Lines 110-210 =="
awk 'NR>=110 && NR<=210 {printf "%d\t%s\n", NR, $0}' "$FILE"
echo
echo "== Search for DiscardUnknown / protojson.UnmarshalOptions usages =="
rg -n "DiscardUnknown|protojson\.UnmarshalOptions|protojson\.Unmarshal" "$FILE" || true
echo
echo "== Search for C-style for loops in this file =="
rg -n "for\s+[a-zA-Z_]+\s*:=\s*0;|for\s+[a-zA-Z_]+\s*:=.*;.*;.*\-\-|for\s+[a-zA-Z_]+\s*:=.*;.*<.*;|for\s+[a-zA-Z_]+\s*:=.*;.*<=.*;|for\s*.*;.*;.*" "$FILE" || trueRepository: NVIDIA/infra-controller
Length of output: 2965
Harden passthrough JSON decoding and align loop style
invokeJSONdecodes withprotojson.UnmarshalOptions{DiscardUnknown: true}, which silently drops unknown/typoed fields and can yield unintended zero values instead of failing fast.listMethodsusesfor i := 0; i < methods.Len(); i++ { ... }; switch to Go’s range-over-integer style.
Suggested change (strict decode)
- if err := (protojson.UnmarshalOptions{DiscardUnknown: true}).Unmarshal(reqJSON, in); err != nil {
+ if err := protojson.Unmarshal(reqJSON, in); err != nil {
return nil, fmt.Errorf("decode request for %q: %w", md.Name(), err)
}Suggested change (loop style)
- for i := 0; i < methods.Len(); i++ {
+ for i := range methods.Len() {
md := methods.Get(i)
// The passthrough only supports unary RPCs.
if md.IsStreamingClient() || md.IsStreamingServer() {
continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/flow/internal/nicoapi/passthrough.go` around lines 137 - 139, Change
the JSON decoding in invokeJSON to be strict (stop discarding unknown fields) so
unknown/typoed fields cause an error instead of silently producing zero values:
replace the current protojson.UnmarshalOptions{DiscardUnknown: true} usage in
invokeJSON with options that do not discard unknown fields (i.e.,
DiscardUnknown: false or omit the option) so Unmarshal fails on unknown fields
and return that error; also update the iteration in listMethods to use Go’s
idiomatic range-over-integer form (use for i := range ... or for i := 0; i <
methods.Len(); i++ -> for i := range make([]struct{}, methods.Len()) style
replacement) to match the repo’s loop style.
| responses: | ||
| '200': | ||
| description: OK | ||
| content: | ||
| application/json: | ||
| schema: | ||
| $ref: '#/components/schemas/NICoCorePassthroughResponse' | ||
| '403': | ||
| $ref: '#/components/responses/ForbiddenError' |
There was a problem hiding this comment.
Document 400 Bad Request status code.
The handler explicitly returns 400 for invalid request body and missing method field (lines 198-199, 201-203 in nicocorepassthrough.go), but the spec only documents 200 and 403 responses. API consumers need to know about client-error conditions.
📝 Suggested addition
responses:
'200':
description: OK
content:
application/json:
schema:
$ref: '`#/components/schemas/NICoCorePassthroughResponse`'
+ '400':
+ $ref: '`#/components/responses/BadRequestError`'
'403':
$ref: '`#/components/responses/ForbiddenError`'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| responses: | |
| '200': | |
| description: OK | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '#/components/schemas/NICoCorePassthroughResponse' | |
| '403': | |
| $ref: '#/components/responses/ForbiddenError' | |
| responses: | |
| '200': | |
| description: OK | |
| content: | |
| application/json: | |
| schema: | |
| $ref: '`#/components/schemas/NICoCorePassthroughResponse`' | |
| '400': | |
| $ref: '`#/components/responses/BadRequestError`' | |
| '403': | |
| $ref: '`#/components/responses/ForbiddenError`' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 342 - 350, The OpenAPI spec for the
NICoCore passthrough endpoint is missing the documented 400 response even though
nicocorepassthrough.go returns 400 for invalid request bodies and missing
"method"; update the spec under the endpoint responses to include a '400'
response (e.g., reference an existing BadRequestError response component or add
a new components/responses/BadRequestError and reference it) so clients see the
client-error condition and match the handler behavior.
| NICoCorePassthroughResponse: | ||
| type: object | ||
| title: NICoCorePassthroughResponse | ||
| description: Result of a NICo Core gRPC passthrough invocation. | ||
| properties: | ||
| method: | ||
| type: string | ||
| description: The bare method that was invoked. | ||
| response: | ||
| type: object | ||
| additionalProperties: true | ||
| description: protojson-encoded response message from NICo Core. |
There was a problem hiding this comment.
Mark response fields as required.
The handler always populates both method and response fields (see handler/nicocorepassthrough.go:225-228), but the schema declares neither as required. This weakens the API contract and may cause OpenAPI generators to produce optional fields in client SDKs when these are guaranteed to be present.
📝 Proposed fix
NICoCorePassthroughResponse:
type: object
title: NICoCorePassthroughResponse
description: Result of a NICo Core gRPC passthrough invocation.
+ required:
+ - method
+ - response
properties:
method:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 12844 - 12855, The OpenAPI schema
NICoCorePassthroughResponse must declare its always-populated fields as
required; update the NICoCorePassthroughResponse object to include a required
array listing "method" and "response" so the spec matches the handler that
always sets those fields (see the NICo Core passthrough handler that populates
method and response).
| NICoCoreMethodsResponse: | ||
| type: object | ||
| title: NICoCoreMethodsResponse | ||
| description: Catalog of invocable NICo Core gRPC methods. | ||
| properties: | ||
| service: | ||
| type: string | ||
| description: Fully qualified gRPC service name. | ||
| methods: | ||
| type: array | ||
| items: | ||
| $ref: '#/components/schemas/NICoCoreMethod' |
There was a problem hiding this comment.
Mark catalog response fields as required.
The handler always populates both service and methods fields (see handler/nicocorepassthrough.go:276-279), but the schema omits the required constraint. This gap affects client code generation—SDKs may treat these as optional when they are structurally guaranteed.
📝 Proposed fix
NICoCoreMethodsResponse:
type: object
title: NICoCoreMethodsResponse
description: Catalog of invocable NICo Core gRPC methods.
+ required:
+ - service
+ - methods
properties:
service:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NICoCoreMethodsResponse: | |
| type: object | |
| title: NICoCoreMethodsResponse | |
| description: Catalog of invocable NICo Core gRPC methods. | |
| properties: | |
| service: | |
| type: string | |
| description: Fully qualified gRPC service name. | |
| methods: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/NICoCoreMethod' | |
| NICoCoreMethodsResponse: | |
| type: object | |
| title: NICoCoreMethodsResponse | |
| description: Catalog of invocable NICo Core gRPC methods. | |
| required: | |
| - service | |
| - methods | |
| properties: | |
| service: | |
| type: string | |
| description: Fully qualified gRPC service name. | |
| methods: | |
| type: array | |
| items: | |
| $ref: '`#/components/schemas/NICoCoreMethod`' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 12856 - 12867, The OpenAPI schema
NICoCoreMethodsResponse is missing a required constraint for fields that are
always set; update the NICoCoreMethodsResponse schema to include a required
array with "service" and "methods" so both properties are marked mandatory
(i.e., add required: ["service","methods"])—this matches the handler that always
populates those fields (see NICoCoreMethodsResponse schema and the nicocore
passthrough handler that constructs the response).
| NICoCoreMethod: | ||
| type: object | ||
| title: NICoCoreMethod | ||
| description: A single invocable NICo Core gRPC method. | ||
| properties: | ||
| method: | ||
| type: string | ||
| fullMethod: | ||
| type: string | ||
| inputType: | ||
| type: string | ||
| outputType: | ||
| type: string | ||
| mutation: | ||
| type: boolean | ||
| deprecated: | ||
| type: boolean |
There was a problem hiding this comment.
Mark core method metadata fields as required.
Every method entry returned from protobuf reflection will have method, fullMethod, inputType, and outputType populated—these are structural metadata, not optional attributes. The mutation and deprecated flags are always computed as well. Declare all six fields as required to ensure client SDKs reflect the actual contract.
📝 Proposed fix
NICoCoreMethod:
type: object
title: NICoCoreMethod
description: A single invocable NICo Core gRPC method.
+ required:
+ - method
+ - fullMethod
+ - inputType
+ - outputType
+ - mutation
+ - deprecated
properties:
method:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rest-api/openapi/spec.yaml` around lines 12868 - 12884, Update the
NICoCoreMethod schema to mark all six properties as required so client SDKs
reflect the actual contract: add a required array containing "method",
"fullMethod", "inputType", "outputType", "mutation", and "deprecated" within the
NICoCoreMethod object definition (the schema named NICoCoreMethod) so those
fields are validated as present.
|
@ajf Sorry! This one is still a draft, I switched it to ready for review to try to get coderabbit to review it earlier when I was having problems triggering it. |
2 participants
No description provided.