chore: Update Core proto snapshot for REST components#251
Merged
thossain-nv merged 13 commits intomainfrom Apr 8, 2026
Merged
Conversation
Test Results8 540 tests - 8 8 540 ✅ - 8 8m 0s ⏱️ -5s Results for commit bdb66e0. ± Comparison against base commit 5fee842. This pull request removes 8 tests.♻️ This comment has been updated with latest results. |
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
d592982 to
600dd39
Compare
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRenamed Makefile proto targets and repo paths, migrated iPXE payloads from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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-03-13 01:41:04 UTC | Commit: 600dd39 |
🛡️ Vulnerability Scan🚨 Found 64 vulnerability(ies) Severity Breakdown:
🔗 View full details in Security tab 🕐 Last updated: 2026-03-13 01:42:36 UTC | Commit: 600dd39 |
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
600dd39 to
97ece6a
Compare
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
5f3a8c8 to
7a38900
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
site-workflow/pkg/grpc/server/forge_test_server.go (1)
485-508:⚠️ Potential issue | 🟠 MajorIncorrect map key check in
CreateIBPartition- usesDefaultNetworkSegmentIdinstead ofDefaultIBParitionId.Line 492 checks
f.ibp[DefaultNetworkSegmentId]when it should checkf.ibp[DefaultIBParitionId]. This copy-paste error causes the function to always generate a new UUID because the network segment ID will never exist in the IBPartition map.Proposed fix
func (f *ForgeServerImpl) CreateIBPartition(c context.Context, req *cwssaws.IBPartitionCreationRequest) (*cwssaws.IBPartition, error) { if req == nil || req.Config == nil { return nil, status.Errorf(codes.InvalidArgument, "Invalid request argument") } nid := DefaultIBParitionId - _, ok := f.ibp[DefaultNetworkSegmentId] + _, ok := f.ibp[DefaultIBParitionId] if ok { // Default IBPartition already exists, create a new one with a different ID nid = uuid.NewString() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-workflow/pkg/grpc/server/forge_test_server.go` around lines 485 - 508, In CreateIBPartition, the map key check uses DefaultNetworkSegmentId instead of the intended DefaultIBParitionId, causing f.ibp lookups to miss the default entry; update the lookup in the CreateIBPartition method (the check on f.ibp[...]) to use DefaultIBParitionId so nid is only replaced with uuid.NewString() when the default IB partition actually exists, leaving the rest of the logic (nibp creation and f.ibp[nid] assignment) unchanged.site-workflow/pkg/grpc/client/subnet.go (1)
148-161:⚠️ Potential issue | 🟡 MinorIncorrect log message and span name in
GetNetworkSegment.The log message on Line 149 and the span name on Line 150 both reference
GetAllNetworkSegments, but this function isGetNetworkSegment. This will cause confusion in logs and distributed traces when debugging.Proposed fix
func (sub *network) GetNetworkSegment(ctx context.Context, request *wflows.UUID) (response *wflows.NetworkSegment, err error) { - log.Info().Interface("request", request).Msg("GetAllNetworkSegments: received request") - ctx, span := otel.Tracer(os.Getenv("LS_SERVICE_NAME")).Start(ctx, "CarbideClient-GetAllNetworkSegments") + log.Info().Interface("request", request).Msg("GetNetworkSegment: received request") + ctx, span := otel.Tracer(os.Getenv("LS_SERVICE_NAME")).Start(ctx, "CarbideClient-GetNetworkSegment") defer span.End()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-workflow/pkg/grpc/client/subnet.go` around lines 148 - 161, The log and tracing span in the network.GetNetworkSegment method are misnamed as "GetAllNetworkSegments"; update the log.Info() call (currently Msg("GetAllNetworkSegments: received request")) to Msg("GetNetworkSegment: received request") and change the otel span name in otel.Tracer(...).Start(...) from "CarbideClient-GetAllNetworkSegments" to "CarbideClient-GetNetworkSegment" so logs and traces correctly reflect the GetNetworkSegment function.
🧹 Nitpick comments (5)
Makefile (2)
230-233: Minor inconsistency:echovs@echoprefix.Lines 230 and 233 use
echowithout the@prefix, which causes Make to print both the command and its output. The majority of informational messages in this Makefile use@echoto suppress command echoing. While this is consistent with the existingrla-protogentarget (line 246), aligning with the dominant pattern improves output clarity.♻️ Proposed fix for consistency
rm -rf nico-core - echo "Successfully copied Core protobuf files. However, updating Core proto is a complex process and requires manual editing of copied files. Check for WARNING statements in the diff." + `@echo` "Successfully copied Core protobuf files. However, updating Core proto is a complex process and requires manual editing of copied files. Check for WARNING statements in the diff." core-protogen: - echo "Generating protobuf for Core" + `@echo` "Generating protobuf for Core" cd workflow-schema && buf generate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 230 - 233, Change the two informational echo commands in the Makefile to use the suppressed form so the command itself isn't printed: replace the plain echo invocations that emit "Successfully copied Core protobuf files..." and the one under the core-protogen target that prints "Generating protobuf for Core" with `@echo` to match the rest of the Makefile (e.g., the rla-protogen pattern).
222-223: Git operations lack error handling;lscommand serves no functional purpose.The
git pulloperation on line 222 may fail silently (e.g., merge conflicts, authentication issues), and execution would continue with potentially stale or inconsistent state. Additionally, thelscommand on line 223 outputs to stdout but the result is neither validated nor used—if the directory does not exist, the error message will be opaque.Consider adding explicit validation:
♻️ Proposed improvement for robustness
core-proto: - if [ -d "nico-core" ]; then cd nico-core && git pull; else git clone ssh://git@github.com/NVIDIA/ncx-infra-controller-core.git nico-core; fi - ls nico-core/crates/rpc/proto + `@if` [ -d "nico-core" ]; then \ + echo "Updating existing nico-core repository..."; \ + cd nico-core && git pull || { echo "ERROR: git pull failed"; exit 1; }; \ + else \ + echo "Cloning nico-core repository..."; \ + git clone ssh://git@github.com/NVIDIA/ncx-infra-controller-core.git nico-core || { echo "ERROR: git clone failed"; exit 1; }; \ + fi + `@if` [ ! -d "nico-core/crates/rpc/proto" ]; then \ + echo "ERROR: Proto directory not found at nico-core/crates/rpc/proto"; \ + exit 1; \ + fi `@for` file in nico-core/crates/rpc/proto/*.proto; do \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 222 - 223, The Makefile currently runs conditional git pull/clone for the "nico-core" directory and then runs `ls nico-core/crates/rpc/proto` without validating outcomes; update this block to add robust error handling by (1) checking the exit status of `git pull` and, on failure, either abort with a clear error or attempt a fresh clone (delete & reclone) to avoid stale state, (2) ensure `git clone` failures are detected and cause the recipe to exit with a nonzero status, and (3) replace the `ls nico-core/crates/rpc/proto` line with an explicit test like `if [ -d "nico-core/crates/rpc/proto" ]; then :; else echo "error: missing nico-core/crates/rpc/proto" >&2; exit 1; fi` so absence is reported and fails the build; ensure all shell commands use `set -e` or explicit `||` checks to prevent silent continuation.site-workflow/pkg/grpc/server/forge_test_server.go (1)
423-440: Include keyset ID in error message for consistency and debuggability.Line 439 returns an error without including the keyset ID, unlike other methods in this file (e.g.,
DeleteTenantKeysetat line 456). Including the ID aids in debugging.Proposed fix
- return nil, status.Errorf(codes.Internal, "TenantKeyset with ID not found") + return nil, status.Errorf(codes.NotFound, "TenantKeyset with ID %q not found", eid)Note: The error code should also be
codes.NotFoundrather thancodes.Internalto match the semantics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@site-workflow/pkg/grpc/server/forge_test_server.go` around lines 423 - 440, The UpdateTenantKeyset function currently returns a generic internal error when the keyset ID is missing; update the error to use codes.NotFound and include the keyset identifier (eid) in the message for consistency with other methods (e.g., DeleteTenantKeyset). Locate UpdateTenantKeyset in ForgeServerImpl, use the existing eid variable and f.tk map check, and change the final return to return nil, status.Errorf(codes.NotFound, "TenantKeyset with ID %s not found", eid).api/pkg/api/handler/instancebatch.go (2)
1858-1869: Consider removing unused variablenoNvlinkDomainMachines.The slice
noNvlinkDomainMachinesis populated at line 1867 but never referenced elsewhere in the function. This constitutes dead code that may cause confusion during future maintenance.♻️ Suggested cleanup
nvlinkDomainMap := make(map[string][]*cdbm.Machine) - noNvlinkDomainMachines := []*cdbm.Machine{} for idx := range machines { machine := &machines[idx] domainID := getNVLinkDomainID(machine) if domainID != "" { nvlinkDomainMap[domainID] = append(nvlinkDomainMap[domainID], machine) - } else { - noNvlinkDomainMachines = append(noNvlinkDomainMachines, machine) } }Alternatively, if machines without NVLink domain information should be handled as a fallback scenario, consider adding a TODO comment documenting the intended future behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 1858 - 1869, The local slice noNvlinkDomainMachines is populated but never used—remove the variable declaration and the branch that appends to it (the else block that appends to noNvlinkDomainMachines) to eliminate dead code in the loop over machines; if you intended to handle machines without an NVLink domain, instead add a TODO comment near the getNVLinkDomainID usage describing the desired fallback behavior and/or add handling logic that uses noNvlinkDomainMachines elsewhere in the function.
1870-1876: Potential improvement: Guard against emptynvlinkDomainMapbefore selection.When
nvlinkDomainMapis empty (all machines lack NVLink domain metadata),bestDomainIDremains an empty string, andnvlinkDomainMap[bestDomainID]returnsnil. The subsequent length check at line 1881 handles this gracefully, but explicit handling would improve readability and provide a more informative error message.♻️ Suggested improvement
+ if len(nvlinkDomainMap) == 0 { + logger.Warn().Int("totalMachines", len(machines)). + Msg("topology optimization enabled but no machines have NVLink domain metadata") + return nil, cutil.NewAPIError(http.StatusConflict, + "Topology optimization requires NVLink domain information, but no machines have domain metadata", nil) + } + // Find the NVLink domain with the most available machines var bestDomainID string for domainID, domainMachines := range nvlinkDomainMap { if len(domainMachines) > len(nvlinkDomainMap[bestDomainID]) { bestDomainID = domainID } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/pkg/api/handler/instancebatch.go` around lines 1870 - 1876, The loop that picks bestDomainID from nvlinkDomainMap doesn't guard against nvlinkDomainMap being empty, leaving bestDomainID as "" and causing a nil lookup; update the logic around nvlinkDomainMap and bestDomainID (the selection block that iterates over nvlinkDomainMap) to first check if len(nvlinkDomainMap) == 0 and return or handle an explicit error (or fallback behavior) with a clear message, otherwise proceed to compute bestDomainID as before, ensuring all callers of the selection handle the error/result appropriately.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site-agent/pkg/components/carbide_test.go`:
- Around line 211-223: The test accesses response.NetworkSegments[0] without
checking the slice length, which can cause a panic; update the test in
carbide_test.go to assert the slice is non-empty before indexing (e.g., use
require.NotEmpty(t, response.NetworkSegments) or require.Greater(t,
len(response.NetworkSegments), 0)) and then assign responseSegment :=
response.NetworkSegments[0]; add the "github.com/stretchr/testify/require"
import if needed so the test fails with a clear assertion instead of panicking.
In `@site-workflow/pkg/grpc/client/instance.go`:
- Around line 144-160: carbideRequest.Config.Os is only created when
request.CustomIpxe is set, but later code unconditionally dereferences
carbideRequest.Config.Os for request.UserData and
request.AlwaysBootWithCustomIpxe, causing a nil-pointer panic; fix by ensuring
carbideRequest.Config.Os is initialized (create a new wflows.OperatingSystem{})
before assigning UserData or RunProvisioningInstructionsOnEveryBoot when either
request.UserData or request.AlwaysBootWithCustomIpxe is non-nil (reuse the same
creation logic used for request.CustomIpxe), then set
carbideRequest.Config.Os.UserData = request.UserData and
carbideRequest.Config.Os.RunProvisioningInstructionsOnEveryBoot =
*request.AlwaysBootWithCustomIpxe as appropriate.
---
Outside diff comments:
In `@site-workflow/pkg/grpc/client/subnet.go`:
- Around line 148-161: The log and tracing span in the network.GetNetworkSegment
method are misnamed as "GetAllNetworkSegments"; update the log.Info() call
(currently Msg("GetAllNetworkSegments: received request")) to
Msg("GetNetworkSegment: received request") and change the otel span name in
otel.Tracer(...).Start(...) from "CarbideClient-GetAllNetworkSegments" to
"CarbideClient-GetNetworkSegment" so logs and traces correctly reflect the
GetNetworkSegment function.
In `@site-workflow/pkg/grpc/server/forge_test_server.go`:
- Around line 485-508: In CreateIBPartition, the map key check uses
DefaultNetworkSegmentId instead of the intended DefaultIBParitionId, causing
f.ibp lookups to miss the default entry; update the lookup in the
CreateIBPartition method (the check on f.ibp[...]) to use DefaultIBParitionId so
nid is only replaced with uuid.NewString() when the default IB partition
actually exists, leaving the rest of the logic (nibp creation and f.ibp[nid]
assignment) unchanged.
---
Nitpick comments:
In `@api/pkg/api/handler/instancebatch.go`:
- Around line 1858-1869: The local slice noNvlinkDomainMachines is populated but
never used—remove the variable declaration and the branch that appends to it
(the else block that appends to noNvlinkDomainMachines) to eliminate dead code
in the loop over machines; if you intended to handle machines without an NVLink
domain, instead add a TODO comment near the getNVLinkDomainID usage describing
the desired fallback behavior and/or add handling logic that uses
noNvlinkDomainMachines elsewhere in the function.
- Around line 1870-1876: The loop that picks bestDomainID from nvlinkDomainMap
doesn't guard against nvlinkDomainMap being empty, leaving bestDomainID as ""
and causing a nil lookup; update the logic around nvlinkDomainMap and
bestDomainID (the selection block that iterates over nvlinkDomainMap) to first
check if len(nvlinkDomainMap) == 0 and return or handle an explicit error (or
fallback behavior) with a clear message, otherwise proceed to compute
bestDomainID as before, ensuring all callers of the selection handle the
error/result appropriately.
In `@Makefile`:
- Around line 230-233: Change the two informational echo commands in the
Makefile to use the suppressed form so the command itself isn't printed: replace
the plain echo invocations that emit "Successfully copied Core protobuf
files..." and the one under the core-protogen target that prints "Generating
protobuf for Core" with `@echo` to match the rest of the Makefile (e.g., the
rla-protogen pattern).
- Around line 222-223: The Makefile currently runs conditional git pull/clone
for the "nico-core" directory and then runs `ls nico-core/crates/rpc/proto`
without validating outcomes; update this block to add robust error handling by
(1) checking the exit status of `git pull` and, on failure, either abort with a
clear error or attempt a fresh clone (delete & reclone) to avoid stale state,
(2) ensure `git clone` failures are detected and cause the recipe to exit with a
nonzero status, and (3) replace the `ls nico-core/crates/rpc/proto` line with an
explicit test like `if [ -d "nico-core/crates/rpc/proto" ]; then :; else echo
"error: missing nico-core/crates/rpc/proto" >&2; exit 1; fi` so absence is
reported and fails the build; ensure all shell commands use `set -e` or explicit
`||` checks to prevent silent continuation.
In `@site-workflow/pkg/grpc/server/forge_test_server.go`:
- Around line 423-440: The UpdateTenantKeyset function currently returns a
generic internal error when the keyset ID is missing; update the error to use
codes.NotFound and include the keyset identifier (eid) in the message for
consistency with other methods (e.g., DeleteTenantKeyset). Locate
UpdateTenantKeyset in ForgeServerImpl, use the existing eid variable and f.tk
map check, and change the final return to return nil,
status.Errorf(codes.NotFound, "TenantKeyset with ID %s not found", eid).
🪄 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: Pro
Run ID: b9054a9a-a694-494b-8182-c78201d86f34
⛔ Files ignored due to path filters (16)
workflow-schema/schema/site-agent/workflows/v1/common_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/dns_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/forge_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/forge_carbide_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/machine_discovery_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/mlx_device_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/nmx_c_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/nmx_c_carbide_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/schema/site-agent/workflows/v1/site_explorer_carbide.pb.gois excluded by!**/*.pb.go,!**/*.pb.goworkflow-schema/site-agent/workflows/v1/common_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/dns_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/forge_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/machine_discovery_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/mlx_device_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/nmx_c_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.protoworkflow-schema/site-agent/workflows/v1/site_explorer_carbide.protois excluded by!workflow-schema/site-agent/workflows/v1/*_carbide.proto
📒 Files selected for processing (21)
Makefileapi/pkg/api/handler/instance.goapi/pkg/api/handler/instancebatch.gosite-agent/pkg/components/carbide_test.gosite-agent/pkg/components/instance_workflow_test.gosite-agent/pkg/components/managers/sshkeygroup/workflow.gosite-workflow/pkg/activity/ibpartition.gosite-workflow/pkg/activity/instance_test.gosite-workflow/pkg/activity/sshkeygroup.gosite-workflow/pkg/activity/subnet.gosite-workflow/pkg/grpc/client/infiniband_partition.gosite-workflow/pkg/grpc/client/instance.gosite-workflow/pkg/grpc/client/machine.gosite-workflow/pkg/grpc/client/sshkeygroup.gosite-workflow/pkg/grpc/client/subnet.gosite-workflow/pkg/grpc/client/testing.gosite-workflow/pkg/grpc/client/vpc.gosite-workflow/pkg/grpc/server/forge_test_server.gosite-workflow/pkg/workflow/instance_test.goworkflow-schema/schema/cloud/interface/cloud_interface.goworkflow-schema/schema/site-agent/interface/workflow_interface.go
💤 Files with no reviewable changes (11)
- site-workflow/pkg/activity/ibpartition.go
- site-workflow/pkg/activity/subnet.go
- site-agent/pkg/components/managers/sshkeygroup/workflow.go
- site-workflow/pkg/grpc/client/machine.go
- site-workflow/pkg/grpc/client/sshkeygroup.go
- site-workflow/pkg/grpc/client/vpc.go
- site-workflow/pkg/grpc/client/infiniband_partition.go
- site-workflow/pkg/activity/sshkeygroup.go
- workflow-schema/schema/site-agent/interface/workflow_interface.go
- site-workflow/pkg/grpc/client/testing.go
- workflow-schema/schema/cloud/interface/cloud_interface.go
Comment on lines
211
to
223
| assert.Nil(t, err) | ||
| assert.NotNil(t, response) | ||
| assert.Equal(t, created.Name, response.Name) | ||
| assert.Equal(t, created.Id.Value, response.Id.Value) | ||
| assert.Equal(t, created.Mtu, response.Mtu) | ||
| assert.Equal(t, created.SubdomainId.Value, response.SubdomainId.Value) | ||
| assert.Equal(t, created.VpcId.Value, response.VpcId.Value) | ||
| assert.Equal(t, len(created.Prefixes), len(response.Prefixes)) | ||
| assert.Equal(t, created.Prefixes[0].Prefix, response.Prefixes[0].Prefix) | ||
| assert.Equal(t, created.Prefixes[0].Gateway, response.Prefixes[0].Gateway) | ||
| assert.Equal(t, created.Prefixes[0].ReserveFirst, response.Prefixes[0].ReserveFirst) | ||
| responseSegment := response.NetworkSegments[0] | ||
| assert.Equal(t, created.Name, responseSegment.Name) | ||
| assert.Equal(t, created.Id.Value, responseSegment.Id.Value) | ||
| assert.Equal(t, created.Mtu, responseSegment.Mtu) | ||
| assert.Equal(t, created.SubdomainId.Value, responseSegment.SubdomainId.Value) | ||
| assert.Equal(t, created.VpcId.Value, responseSegment.VpcId.Value) | ||
| assert.Equal(t, len(created.Prefixes), len(responseSegment.Prefixes)) | ||
| assert.Equal(t, created.Prefixes[0].Prefix, responseSegment.Prefixes[0].Prefix) | ||
| assert.Equal(t, created.Prefixes[0].Gateway, responseSegment.Prefixes[0].Gateway) | ||
| assert.Equal(t, created.Prefixes[0].ReserveFirst, responseSegment.Prefixes[0].ReserveFirst) | ||
| t.Log("GRPCResponse", response) |
Contributor
There was a problem hiding this comment.
Add bounds check before accessing slice element to prevent panic.
Line 213 accesses response.NetworkSegments[0] without verifying the slice is non-empty. If the server returns an empty list (which would indicate a test regression), this will panic rather than fail gracefully with a descriptive assertion error.
Proposed fix using `require`
assert.Nil(t, err)
assert.NotNil(t, response)
+ require.NotEmpty(t, response.NetworkSegments, "expected at least one network segment in response")
responseSegment := response.NetworkSegments[0]
assert.Equal(t, created.Name, responseSegment.Name)Note: You may need to add "github.com/stretchr/testify/require" to imports.
As per coding guidelines, use testify (assert/require) for test assertions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@site-agent/pkg/components/carbide_test.go` around lines 211 - 223, The test
accesses response.NetworkSegments[0] without checking the slice length, which
can cause a panic; update the test in carbide_test.go to assert the slice is
non-empty before indexing (e.g., use require.NotEmpty(t,
response.NetworkSegments) or require.Greater(t, len(response.NetworkSegments),
0)) and then assign responseSegment := response.NetworkSegments[0]; add the
"github.com/stretchr/testify/require" import if needed so the test fails with a
clear assertion instead of panicking.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@site-workflow/pkg/grpc/client/instance.go`:
- Around line 146-160: The CreateInstance handler is missing mapping for
request.PhoneHomeEnabled which causes silent data loss; update the mapping in
the same block that sets OS fields (near where CustomIpxe, UserData, and
AlwaysBootWithCustomIpxe are handled) so that
carbideRequest.Config.Os.PhoneHomeEnabled is assigned from
request.PhoneHomeEnabled (and ensure the types match/unpack pointers as done
elsewhere) so the OperatingSystem.phone_home_enabled field is forwarded to the
carbide service.
🪄 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: Pro
Run ID: 3d4e83fb-2f66-44cc-a8ec-6ef068b3d9d0
📒 Files selected for processing (1)
site-workflow/pkg/grpc/client/instance.go
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
a9c0eb8 to
d854c97
Compare
thossain-nv
commented
Apr 6, 2026
| reserved 9; // host_nics in carbide-core, not used in REST | ||
| repeated ExpectedHostNic host_nics = 9; | ||
| optional common.RackId rack_id = 10; | ||
| // WARNING: Conflict with Core - optional bool default_pause_ingestion_and_poweron = 11; |
Contributor
Author
There was a problem hiding this comment.
@chet Can we please resolve Core proto differences before these changes (v1.3.0) hit Production? Otherwise ExpectedMachine inventory might break in Prod.
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
5832527 to
a76859e
Compare
thossain-nv
requested review from
hwadekar-nv,
kfelternv,
parmani-nv and
pbreton
pbreton
reviewed
Apr 8, 2026
| rm -rf workflow-schema/schema/site-agent/workflows/v1/*.pb.go | ||
|
|
||
| core-proto-fetch: | ||
| if [ -d "nico-core" ]; then cd nico-core && git fetch origin && git reset --hard origin/main; else git clone ssh://git@github.com/NVIDIA/ncx-infra-controller-core.git nico-core; fi |
Contributor
There was a problem hiding this comment.
What about allowing to use a custom 'nico-core' (using a link) with something like:
[ -h "nico-core" ] || if [ -d "nico-core" ]; then cd nico-core ... else git clone ...; fi
Contributor
Author
There was a problem hiding this comment.
That seems helpful. Let me look into it.
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
6177b94 to
9d0befc
Compare
thossain-nv
commented
Apr 8, 2026
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
…roup Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
…ed merge Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
Signed-off-by: Tareque Hossain <thossain@nvidia.com>
…EST snapshot Signed-off-by: Tareque Hossain <thossain@nvidia.com>
thossain-nv
force-pushed
the
chore/update-core-proto
branch
from
9d0befc to
bdb66e0
Compare
hwadekar-nv
approved these changes
Apr 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Snapshotting Core proto files have been a complex process with hand edits until now. This PR introduces an idempotent script in
workflow-schema/cmd/core-proto-fmt/main.gothat can be run anytime to fetch latest snapshot of Core proto files. The script accommodates various idiosyncrasies:Run
make core-prototo take latest snapshotPR also updates REST snapshot to latest proto files from Core
Several non-paginated object retrieval methods were removed from proto, PR also removes the corresponding fallback methods from Site Agent. All objects now have paginated inventory methods in gRPC so fallbacks are no longer needed.
Type of Change
Services Affected
Related Issues (Optional)
None
Breaking Changes
Testing
Additional Notes
None