DP to CP flow for AI workspace and AI Gateway#2219
Open
thivindu wants to merge 8 commits into
Open
Conversation
thivindu
requested review from
AnuGayan,
Arshardh,
CrowleyRajapakse,
HeshanSudarshana,
HiranyaKavishani,
Krishanx92,
PasanT9,
RakhithaRR,
Tharsanan1,
VirajSalaka,
ashera96,
chamilaadhi,
dushaniw,
hisanhunais,
malinthaprasan,
pubudu538,
renuka-fernando,
senthuran16,
tgtshanika,
tharikaGitHub and
tharindu1st
as code owners
Contributor
📝 WalkthroughSummaryThis PR unifies the end-to-end synchronization of artifacts created on the AI Gateway data plane (DP) into the AI Workspace control plane (CP). It replaces API-deployment-specific plumbing with a single gateway→control-plane artifact import/push mechanism, adds full lifecycle handling (deploy/undeploy), and introduces origin/readonly + metadata ownership tracking so CP can enforce correct mutability rules for DP-originated artifacts. Core ChangesUnified DP→CP artifact push + undeploy
LLM provider template DP→CP propagation
Control plane client sync bookkeeping
Control-plane artifact import (platform-api)New generic DP artifact import endpoint
ArtifactImportService + per-kind importers
LLM policy lifting
Origin / readonly + metadata ownershipArtifact origin tracking
Mutability enforcement for DP-originated artifacts
Metadata synchronization semantics
Supporting updates
WalkthroughThis change replaces gateway deployment-specific sync with a generic artifact import flow between the gateway controller and platform internal API. It adds artifact origin tracking, gateway Sequence Diagram(s)sequenceDiagram
participant Gateway
participant GatewayController
participant PlatformInternalAPI
participant ArtifactImportService
participant PlatformDB
Gateway->>GatewayController: create/update/delete artifact
GatewayController->>PlatformInternalAPI: POST /artifacts/import-gateway-artifact
PlatformInternalAPI->>ArtifactImportService: Import(request)
ArtifactImportService->>PlatformDB: create/update artifact and deployment state
PlatformDB-->>ArtifactImportService: stored artifact and deployment
ArtifactImportService-->>PlatformInternalAPI: import response
PlatformInternalAPI-->>GatewayController: control-plane artifact UUID
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
thivindu
removed request for
HeshanSudarshana,
HiranyaKavishani,
PasanT9,
Tharsanan1,
ashera96 and
hisanhunais
![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: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
platform-api/src/internal/service/websub_api_deployment.go (1)
266-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the loaded WebSub API origin for the undeploy guard.
The current guard is skipped when
artifactRepois nil, butwebsubAPI.Originis available from the model loaded below. Move the check after the API lookup so the undeploy path consistently enforces the read-only rule.Proposed fix
- // DP-originated artifacts are read-only in the control plane: their deploy/undeploy - // lifecycle is owned by the data-plane gateway, so the control plane must not - // initiate an undeployment for them. - if s.artifactRepo != nil { - artifact, err := s.artifactRepo.GetByUUID(apiUUID, orgID) - if err != nil { - return nil, fmt.Errorf("failed to look up artifact origin: %w", err) - } - if artifact != nil { - if err := ensureOriginMutable(artifact.Origin); err != nil { - return nil, err - } - } - } - websubAPI, err := s.websubAPIRepo.GetByUUID(apiUUID, orgID) if err != nil { return nil, err } if websubAPI == nil { return nil, constants.ErrWebSubAPINotFound } + // DP-originated artifacts are read-only in the control plane: their deploy/undeploy + // lifecycle is owned by the data-plane gateway, so the control plane must not + // initiate an undeployment for them. + if err := ensureOriginMutable(websubAPI.Origin); err != nil { + return nil, err + }🤖 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 `@platform-api/src/internal/service/websub_api_deployment.go` around lines 266 - 287, The origin mutability check that calls ensureOriginMutable is currently guarded by a nil check on s.artifactRepo and will be skipped if it's nil, but the websubAPI object loaded from s.websubAPIRepo.GetByUUID also contains an Origin property that can be used. Move the ensureOriginMutable check to execute after the websubAPI is loaded and validated (after checking if websubAPI is nil), and have it reference websubAPI.Origin instead of artifact.Origin. This ensures the read-only rule is consistently enforced for the undeploy path regardless of whether artifactRepo is available.platform-api/src/internal/service/webbroker_api_deployment.go (1)
266-287:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the loaded WebBroker API origin for the undeploy guard.
The current guard is skipped when
artifactRepois nil, butwebbrokerAPI.Originis loaded immediately after. Move the check after the API lookup so the undeploy path consistently enforces the read-only rule.Proposed fix
- // DP-originated artifacts are read-only in the control plane: their deploy/undeploy - // lifecycle is owned by the data-plane gateway, so the control plane must not - // initiate an undeployment for them. - if s.artifactRepo != nil { - artifact, err := s.artifactRepo.GetByUUID(apiUUID, orgID) - if err != nil { - return nil, fmt.Errorf("failed to look up artifact origin: %w", err) - } - if artifact != nil { - if err := ensureOriginMutable(artifact.Origin); err != nil { - return nil, err - } - } - } - webbrokerAPI, err := s.webbrokerAPIRepo.GetByUUID(apiUUID, orgID) if err != nil { return nil, err } if webbrokerAPI == nil { return nil, constants.ErrWebBrokerAPINotFound } + // DP-originated artifacts are read-only in the control plane: their deploy/undeploy + // lifecycle is owned by the data-plane gateway, so the control plane must not + // initiate an undeployment for them. + if err := ensureOriginMutable(webbrokerAPI.Origin); err != nil { + return nil, err + }🤖 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 `@platform-api/src/internal/service/webbroker_api_deployment.go` around lines 266 - 287, The undeploy guard using ensureOriginMutable is currently only executed when s.artifactRepo is not nil, causing inconsistent enforcement. After the webbrokerAPI is loaded via s.webbrokerAPIRepo.GetByUUID and its nil check, apply ensureOriginMutable to webbrokerAPI.Origin instead of artifact.Origin to ensure the read-only undeploy rule is consistently enforced regardless of whether s.artifactRepo is available.
🧹 Nitpick comments (3)
gateway/gateway-controller/pkg/utils/api_utils_test.go (1)
288-300: ⚡ Quick winAdd a label-based project metadata success case to this test suite.
Current coverage validates annotation-based project metadata and missing-annotation failure, but it does not verify that label-based project metadata is accepted for project-scoped kinds.
Also applies to: 322-349
🤖 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 `@gateway/gateway-controller/pkg/utils/api_utils_test.go` around lines 288 - 300, The test suite currently validates that org-level kinds omit project annotations and covers failure cases, but lacks a test case for the success case where label-based project metadata is accepted for project-scoped kinds. Add a new test case using t.Run (similar to the existing "Org-level kind omits project" test) that verifies project-scoped kinds correctly accept and handle label-based project metadata. This test should validate that labels containing project information (such as commonconstants.AnnotationProjectID in the labels map rather than annotations) are properly processed and accepted for project-scoped artifact imports.platform-api/src/internal/service/deployment_undeploy_guard_test.go (1)
44-73: ⚡ Quick winAdd a focused DP-origin guard test for
DeployAPI.This file covers restore/undeploy guard paths, but the new deploy guard in
DeploymentService.DeployAPIdoes not appear to have direct unit coverage here. Adding that test will reduce regression risk for the new control-plane block path.🤖 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 `@platform-api/src/internal/service/deployment_undeploy_guard_test.go` around lines 44 - 73, Add a new test function following the existing pattern of TestUndeployDeployment_BlockedForDPOrigin and TestRestoreDeployment_BlockedForDPOrigin. Create a test that verifies the DeployAPI method of DeploymentService also blocks deployment of data-plane-originated artifacts by returning constants.ErrArtifactReadOnly. Set up a DeploymentService with an artifact having Origin set to constants.OriginDP, call DeployAPI with appropriate parameters, and assert the error matches the expected read-only error.platform-api/src/internal/model/mcp.go (1)
37-39: ⚡ Quick winAlign the
Origincomment with current read behavior.The note on Line 38 says
Originis not scanned back, but MCP repository reads now populatep.Originfromartifacts.origin. Updating this comment will avoid confusion.Proposed comment update
- // Origin carries the artifact origin ("CP" or "DP") into create. Stored on the - // artifacts table, not mcp_proxies, so it is not scanned back here. + // Origin carries the artifact origin ("CP" or "DP") into create. It is stored on + // the artifacts table (not mcp_proxies) and populated from artifacts on reads. Origin string `json:"origin,omitempty" db:"-"`🤖 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 `@platform-api/src/internal/model/mcp.go` around lines 37 - 39, Update the comment for the Origin field in the MCP model struct to reflect that it is now scanned back from the database. The current comment states that Origin is not scanned back, but the MCP repository now populates the Origin field from the artifacts.origin column during reads. Revise the comment to accurately describe that Origin is stored on the artifacts table and is read back into the Origin field during database queries, removing the outdated statement about it not being scanned.
🤖 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 `@gateway/gateway-controller/pkg/controlplane/client.go`:
- Around line 4129-4131: In the UpdateCPSyncStatus call within the success path,
the code unconditionally persists the cpArtifactID parameter even when it may be
empty, which can overwrite a previously known mapping in the database. Modify
the logic to preserve the existing CP artifact ID by using it as a fallback when
the cpArtifactID from the response is empty. Check if cpArtifactID is empty and,
if so, substitute it with the pre-existing or previously known artifact ID
before calling UpdateCPSyncStatus to ensure the database mapping is not lost
when the success response lacks an ID.
In `@gateway/gateway-controller/pkg/controlplane/push_artifacts_test.go`:
- Around line 134-147: The REST artifact being seeded in the test via
client.db.SaveConfig with models.KindRestApi is incomplete and missing required
project metadata/configuration. This causes the push to fail before any HTTP
call, making the hit == false assertion unreliable for testing the
deployment_push_enabled gating. Add all required project metadata and
configuration fields to the StoredConfig struct being saved so it creates a
valid, pushable REST fixture that would actually trigger an HTTP call if the
gating is broken.
In `@gateway/gateway-controller/pkg/utils/api_utils.go`:
- Around line 988-1002: The hasProjectAnnotation function only checks for the
project ID in annotations, but the import contract supports project metadata
from both labels and annotations. Update the function to check both the
annotations and labels fields within metadata for the project ID constant,
returning true if the project ID is found in either source. Additionally, check
if there are similar functions around lines 1018-1028 that need the same update
to accept project metadata from either labels or annotations.
In `@platform-api/src/internal/handler/gateway_internal.go`:
- Around line 155-174: In the error handling switch statement within the
artifactImportService.Import handler, there are duplicate cases for
constants.ErrProjectNotFound which makes one unreachable. Additionally, the
current implementation returns a 404 status for missing projects, but the
internal API contract specifies that missing project imports should be treated
as a no-op and return 200. Remove the duplicate ErrProjectNotFound case and
modify the remaining one to return an HTTP 200 status with a no-op success
response instead of 404, ensuring the handler aligns with the documented API
contract for non-existent project imports.
In `@platform-api/src/internal/model/gateway.go`:
- Around line 37-40: The SyncMetadata field defines a single-owner invariant
where only one gateway in an organization should have this flag set to true, but
this constraint is not being enforced in the code. When a gateway's SyncMetadata
field is set to true during create or update operations, you need to clear the
SyncMetadata flag from all other gateways in the same organization to maintain
the invariant. Alternatively, implement this uniqueness constraint at the
repository or database layer using appropriate constraints or validation logic.
Review all code paths that modify the SyncMetadata field on the gateway model
and ensure that whenever it is set to true, any previously enabled
metadata-owner flags in the organization are cleared.
In `@platform-api/src/internal/service/artifact_guard.go`:
- Around line 62-67: The ensureOriginDeletable function calls
deploymentRepo.HasActiveDeployment without checking if deploymentRepo is nil,
which can cause a panic if an uninitialized repository is passed in. Add a nil
check for the deploymentRepo parameter at the start of the function (or
immediately before the HasActiveDeployment call) and return a controlled error
with an appropriate message if the repository is nil, rather than allowing
execution to proceed and panic.
In `@platform-api/src/internal/service/artifact_import_llm_provider.go`:
- Around line 89-93: The error handling for the resolveTemplateUUID call is
insufficient and allows the import to proceed even when template resolution
fails. Currently, the code only updates existing.TemplateUUID when err is nil,
but ignores the error case, which can leave the provider configuration in an
inconsistent state. Instead of ignoring the error, modify the code to return an
error from the current function when resolveTemplateUUID fails, ensuring the
entire import operation fails and prevents the inconsistent state.
In `@platform-api/src/internal/service/artifact_import_llm_proxy.go`:
- Around line 94-106: The provider resolution is currently conditional on
cfg.Provider being non-empty, which allows empty provider values to skip
validation while still replacing existing.Configuration, creating inconsistent
state between ProviderUUID and the persisted Configuration. Remove the
conditional check on cfg.Provider and resolve the provider unconditionally
within the shouldWriteMetadata block by moving the call to i.resolveProviderUUID
outside of the cfg.Provider != "" condition so that provider validation occurs
regardless of whether cfg.Provider is empty, ensuring invalid proxy specs fail
rather than creating inconsistent state.
In `@platform-api/src/internal/service/artifact_import_rest.go`:
- Line 50: The comment near the artifact import logic in artifact_import_rest.go
incorrectly describes ctx.ID as the gateway UUID when it actually represents the
CP-resolved artifact UUID. Update the comment to accurately reflect that ctx.ID
is the CP-resolved artifact UUID and ensure the comment aligns with the import
contract rather than describing it as preserving the gateway UUID.
- Around line 70-84: The current code creates a partial model.API object and
sends it to UpdateAPI, which risks resetting fields that are not part of the
import. Instead, load the existing API first (using a repository method or from
available context), then mutate only the fields that are owned by the import
process (such as Name, Version, and Configuration), while preserving all other
existing fields. Apply this safer pattern that matches the MCP importer approach
to ensure non-imported fields are not inadvertently reset during metadata-sync
updates.
In `@platform-api/src/internal/service/artifact_import_test.go`:
- Around line 66-71: The PRAGMA foreign_keys setting in the sqlDB.Exec call only
applies to the current SQLite connection, but sql.Open creates a connection pool
that may use different connections for subsequent queries. Fix this by either
adding sqlDB.SetMaxOpenConns(1) immediately after the sql.Open call to ensure
all operations use a single connection, or alternatively modify the DSN string
parameter passed to sql.Open to include _foreign_keys=ON as a query parameter so
foreign keys are enforced at the driver level for all connections.
In `@platform-api/src/internal/service/artifact_import.go`:
- Around line 266-267: The mapDeploymentStatus function silently defaults
unknown req.Status values to DeploymentStatusDeployed, which can persist
incorrect deployment states. Modify the mapDeploymentStatus function to return
an error for unknown status values, then update the code at the point of
invocation (around line 266 where status is assigned from
mapDeploymentStatus(ictx.Status)) to check for this error and reject the
operation instead of proceeding with an invalid state. Apply the same validation
pattern to the other location mentioned at lines 340-353.
In `@platform-api/src/resources/gateway-internal-api.yaml`:
- Around line 1508-1514: The id field documented as a UUID is missing the format
constraint in the OpenAPI schema. Add format: uuid to the id field definition in
the data-plane artifact UUID section (shown in the diff around line 1508-1514),
and also apply the same format constraint to any other id fields in the schema.
Additionally, in the ImportGatewayArtifactResponse definition (referenced at
lines 1610-1638), make the id, status, and origin fields required properties and
add an enum constraint to the origin field to restrict it to the value DP only.
- Around line 1213-1215: There is a contract mismatch in the
gateway-internal-api.yaml file between the documented behavior and the response
codes: the description around lines 1213-1215 states that a missing project
returns a 200 no-op response, but the response table documents a 404 status code
for project not found. Decide which behavior is correct for the project import
endpoint, then update both the description text and the corresponding response
table entry to be consistent with that behavior. Also apply the same correction
to the related section at lines 1244-1245 to ensure all project-not-found
scenarios are handled consistently throughout the specification.
- Around line 1555-1575: There is a contradiction between the description text
for the `kind` field and its actual schema constraint. The description claims
new artifact kinds can be added without changing the contract, but the `kind`
field has a closed enum restricting it to specific values (RestApi, LlmProvider,
LlmProviderTemplate, LlmProxy, Mcp). Either remove the enum constraint entirely
if kinds are truly extensible without contract changes, or update the
descriptive text to explicitly state that adding new kinds requires updating
this schema and enum list.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 11485-11488: The readOnly field in the WebSubAPI and WebBrokerAPI
schemas is missing the readOnly: true keyword that is present in other artifact
schemas. Add readOnly: true to the readOnly field definition in both WebSubAPI
(around lines 11485-11488) and WebBrokerAPI (around lines 11703-11706) to mark
this as a server-owned field so that generated request models do not allow
clients to set this origin-derived state. This ensures consistency with other
artifact schemas in the OpenAPI specification.
- Around line 8346-8355: The RegisterGateway and UpdateGateway methods allow
multiple gateways in the same organization to have syncMetadata=true
simultaneously, violating the documented constraint that only one gateway per
organization should own metadata sync. Fix this by wrapping both the gateway
creation (RegisterGateway) and update (UpdateGateway) operations in a database
transaction that atomically clears the syncMetadata flag on any existing owner
gateway in the organization before setting syncMetadata=true on the new gateway,
or alternatively enforce single ownership through a database constraint that
prevents multiple gateways with syncMetadata=true in the same organization.
---
Outside diff comments:
In `@platform-api/src/internal/service/webbroker_api_deployment.go`:
- Around line 266-287: The undeploy guard using ensureOriginMutable is currently
only executed when s.artifactRepo is not nil, causing inconsistent enforcement.
After the webbrokerAPI is loaded via s.webbrokerAPIRepo.GetByUUID and its nil
check, apply ensureOriginMutable to webbrokerAPI.Origin instead of
artifact.Origin to ensure the read-only undeploy rule is consistently enforced
regardless of whether s.artifactRepo is available.
In `@platform-api/src/internal/service/websub_api_deployment.go`:
- Around line 266-287: The origin mutability check that calls
ensureOriginMutable is currently guarded by a nil check on s.artifactRepo and
will be skipped if it's nil, but the websubAPI object loaded from
s.websubAPIRepo.GetByUUID also contains an Origin property that can be used.
Move the ensureOriginMutable check to execute after the websubAPI is loaded and
validated (after checking if websubAPI is nil), and have it reference
websubAPI.Origin instead of artifact.Origin. This ensures the read-only rule is
consistently enforced for the undeploy path regardless of whether artifactRepo
is available.
---
Nitpick comments:
In `@gateway/gateway-controller/pkg/utils/api_utils_test.go`:
- Around line 288-300: The test suite currently validates that org-level kinds
omit project annotations and covers failure cases, but lacks a test case for the
success case where label-based project metadata is accepted for project-scoped
kinds. Add a new test case using t.Run (similar to the existing "Org-level kind
omits project" test) that verifies project-scoped kinds correctly accept and
handle label-based project metadata. This test should validate that labels
containing project information (such as commonconstants.AnnotationProjectID in
the labels map rather than annotations) are properly processed and accepted for
project-scoped artifact imports.
In `@platform-api/src/internal/model/mcp.go`:
- Around line 37-39: Update the comment for the Origin field in the MCP model
struct to reflect that it is now scanned back from the database. The current
comment states that Origin is not scanned back, but the MCP repository now
populates the Origin field from the artifacts.origin column during reads. Revise
the comment to accurately describe that Origin is stored on the artifacts table
and is read back into the Origin field during database queries, removing the
outdated statement about it not being scanned.
In `@platform-api/src/internal/service/deployment_undeploy_guard_test.go`:
- Around line 44-73: Add a new test function following the existing pattern of
TestUndeployDeployment_BlockedForDPOrigin and
TestRestoreDeployment_BlockedForDPOrigin. Create a test that verifies the
DeployAPI method of DeploymentService also blocks deployment of
data-plane-originated artifacts by returning constants.ErrArtifactReadOnly. Set
up a DeploymentService with an artifact having Origin set to constants.OriginDP,
call DeployAPI with appropriate parameters, and assert the error matches the
expected read-only error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d537fe82-93ca-41e4-9524-2532fba3cef1
📒 Files selected for processing (88)
gateway/build-manifest.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/handlers_test.gogateway/gateway-controller/pkg/api/handlers/llm_provider_handler.gogateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.gogateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/rest_api_handler.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/controlplane_test.gogateway/gateway-controller/pkg/controlplane/push_artifacts_test.gogateway/gateway-controller/pkg/controlplane/sync.gogateway/gateway-controller/pkg/models/stored_config.gogateway/gateway-controller/pkg/service/restapi/service.gogateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/utils/api_deployment_test.gogateway/gateway-controller/pkg/utils/api_key_test.gogateway/gateway-controller/pkg/utils/api_utils.gogateway/gateway-controller/pkg/utils/api_utils_test.goplatform-api/oapi-codegen.yamlplatform-api/src/api/generated.goplatform-api/src/internal/constants/constants.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/dto/artifact_import.goplatform-api/src/internal/dto/gateway_internal.goplatform-api/src/internal/handler/api.goplatform-api/src/internal/handler/api_deployment.goplatform-api/src/internal/handler/artifact_guard_response.goplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/handler/llm.goplatform-api/src/internal/handler/llm_deployment.goplatform-api/src/internal/handler/mcp.goplatform-api/src/internal/handler/mcp_deployment.goplatform-api/src/internal/handler/webbroker_api.goplatform-api/src/internal/handler/webbroker_api_deployment.goplatform-api/src/internal/handler/websub_api.goplatform-api/src/internal/handler/websub_api_deployment.goplatform-api/src/internal/model/api.goplatform-api/src/internal/model/artifact.goplatform-api/src/internal/model/gateway.goplatform-api/src/internal/model/llm.goplatform-api/src/internal/model/mcp.goplatform-api/src/internal/model/webbroker_api.goplatform-api/src/internal/model/websub_api.goplatform-api/src/internal/repository/api.goplatform-api/src/internal/repository/artifact.goplatform-api/src/internal/repository/deployment.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/mcp.goplatform-api/src/internal/repository/webbroker_api.goplatform-api/src/internal/repository/websub_api.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/api.goplatform-api/src/internal/service/artifact_cross_origin_test.goplatform-api/src/internal/service/artifact_guard.goplatform-api/src/internal/service/artifact_import.goplatform-api/src/internal/service/artifact_import_lifecycle_test.goplatform-api/src/internal/service/artifact_import_llm_policies.goplatform-api/src/internal/service/artifact_import_llm_policies_test.goplatform-api/src/internal/service/artifact_import_llm_provider.goplatform-api/src/internal/service/artifact_import_llm_proxy.goplatform-api/src/internal/service/artifact_import_llm_template.goplatform-api/src/internal/service/artifact_import_mcp.goplatform-api/src/internal/service/artifact_import_rest.goplatform-api/src/internal/service/artifact_import_test.goplatform-api/src/internal/service/artifact_readonly_test.goplatform-api/src/internal/service/deployment.goplatform-api/src/internal/service/deployment_undeploy_guard_test.goplatform-api/src/internal/service/gateway.goplatform-api/src/internal/service/gateway_internal.goplatform-api/src/internal/service/gateway_properties_test.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_deployment.goplatform-api/src/internal/service/mcp.goplatform-api/src/internal/service/mcp_deployment.goplatform-api/src/internal/service/webbroker_api.goplatform-api/src/internal/service/webbroker_api_deployment.goplatform-api/src/internal/service/websub_api.goplatform-api/src/internal/service/websub_api_deployment.goplatform-api/src/internal/utils/api.goplatform-api/src/resources/gateway-internal-api.yamlplatform-api/src/resources/openapi.yaml
💤 Files with no reviewable changes (1)
- platform-api/src/internal/service/gateway_internal.go
thivindu
force-pushed
the
ai-workspace-dp-cp
branch
from
a733126 to
ae8d3ab
Compare
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 (1)
docs/rest-apis/gateway/webbroker-api-management.md (1)
801-807:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the request description with the payload example.
Line 801 describes setting a custom key value, but the example on Line 807 only sends
name. Please either include the custom-value field fromAPIKeyUpdateRequestin the payload example or reword the description to match a metadata-only update.🤖 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 `@docs/rest-apis/gateway/webbroker-api-management.md` around lines 801 - 807, The description on line 801 states the API key is being updated with a custom value instead of auto-generating one, but the JSON payload example starting at line 807 only includes the name field and is missing the custom-value field that would correspond to the description. Either add the custom-value field to the JSON payload example to match the description of setting a custom key value, or reword the description to accurately reflect that the payload only updates the name metadata without setting a custom value.
🤖 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 `@docs/rest-apis/gateway/websub-api-management.md`:
- Around line 799-803: The example response message for the webhook secret
regenerate endpoint is incorrect. In the JSON response example for the
regenerate operation, the message field currently states "Webhook secret
generated successfully" but should say "Webhook secret regenerated successfully"
to accurately reflect that the endpoint is regenerating an existing secret, not
generating a new one for the first time. Update the message string value in the
response example to use the past tense "regenerated" instead of "generated".
- Around line 858-870: The 404 response example in the WebSub API documentation
shows a configuration-validation error payload which does not match the actual
error responses for this endpoint. Replace the entire 404 response example with
a proper WebSub secret-not-found or WebSub API-not-found style error response
that accurately reflects what would be returned when a WebSub API or secret is
missing or not found on this endpoint.
---
Outside diff comments:
In `@docs/rest-apis/gateway/webbroker-api-management.md`:
- Around line 801-807: The description on line 801 states the API key is being
updated with a custom value instead of auto-generating one, but the JSON payload
example starting at line 807 only includes the name field and is missing the
custom-value field that would correspond to the description. Either add the
custom-value field to the JSON payload example to match the description of
setting a custom key value, or reword the description to accurately reflect that
the payload only updates the name metadata without setting a custom value.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5def23a7-8e4e-4853-b9e6-67d6d4f450a3
📒 Files selected for processing (92)
docs/rest-apis/gateway/README.mddocs/rest-apis/gateway/schemas.mddocs/rest-apis/gateway/webbroker-api-management.mddocs/rest-apis/gateway/websub-api-management.mdgateway/build-manifest.yamlgateway/gateway-controller/pkg/api/handlers/handlers.gogateway/gateway-controller/pkg/api/handlers/handlers_test.gogateway/gateway-controller/pkg/api/handlers/llm_provider_handler.gogateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.gogateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.gogateway/gateway-controller/pkg/api/handlers/rest_api_handler.gogateway/gateway-controller/pkg/controlplane/client.gogateway/gateway-controller/pkg/controlplane/controlplane_test.gogateway/gateway-controller/pkg/controlplane/push_artifacts_test.gogateway/gateway-controller/pkg/controlplane/sync.gogateway/gateway-controller/pkg/models/stored_config.gogateway/gateway-controller/pkg/service/restapi/service.gogateway/gateway-controller/pkg/storage/sql_store.gogateway/gateway-controller/pkg/utils/api_deployment_test.gogateway/gateway-controller/pkg/utils/api_key_test.gogateway/gateway-controller/pkg/utils/api_utils.gogateway/gateway-controller/pkg/utils/api_utils_test.goplatform-api/oapi-codegen.yamlplatform-api/src/api/generated.goplatform-api/src/internal/constants/constants.goplatform-api/src/internal/constants/error.goplatform-api/src/internal/database/schema.postgres.sqlplatform-api/src/internal/database/schema.sqlplatform-api/src/internal/database/schema.sqlite.sqlplatform-api/src/internal/dto/artifact_import.goplatform-api/src/internal/dto/gateway_internal.goplatform-api/src/internal/handler/api.goplatform-api/src/internal/handler/api_deployment.goplatform-api/src/internal/handler/artifact_guard_response.goplatform-api/src/internal/handler/gateway.goplatform-api/src/internal/handler/gateway_internal.goplatform-api/src/internal/handler/llm.goplatform-api/src/internal/handler/llm_deployment.goplatform-api/src/internal/handler/mcp.goplatform-api/src/internal/handler/mcp_deployment.goplatform-api/src/internal/handler/webbroker_api.goplatform-api/src/internal/handler/webbroker_api_deployment.goplatform-api/src/internal/handler/websub_api.goplatform-api/src/internal/handler/websub_api_deployment.goplatform-api/src/internal/model/api.goplatform-api/src/internal/model/artifact.goplatform-api/src/internal/model/gateway.goplatform-api/src/internal/model/llm.goplatform-api/src/internal/model/mcp.goplatform-api/src/internal/model/webbroker_api.goplatform-api/src/internal/model/websub_api.goplatform-api/src/internal/repository/api.goplatform-api/src/internal/repository/artifact.goplatform-api/src/internal/repository/deployment.goplatform-api/src/internal/repository/gateway.goplatform-api/src/internal/repository/interfaces.goplatform-api/src/internal/repository/llm.goplatform-api/src/internal/repository/mcp.goplatform-api/src/internal/repository/webbroker_api.goplatform-api/src/internal/repository/websub_api.goplatform-api/src/internal/server/server.goplatform-api/src/internal/service/api.goplatform-api/src/internal/service/artifact_cross_origin_test.goplatform-api/src/internal/service/artifact_guard.goplatform-api/src/internal/service/artifact_import.goplatform-api/src/internal/service/artifact_import_lifecycle_test.goplatform-api/src/internal/service/artifact_import_llm_policies.goplatform-api/src/internal/service/artifact_import_llm_policies_test.goplatform-api/src/internal/service/artifact_import_llm_provider.goplatform-api/src/internal/service/artifact_import_llm_proxy.goplatform-api/src/internal/service/artifact_import_llm_template.goplatform-api/src/internal/service/artifact_import_mcp.goplatform-api/src/internal/service/artifact_import_rest.goplatform-api/src/internal/service/artifact_import_test.goplatform-api/src/internal/service/artifact_readonly_test.goplatform-api/src/internal/service/deployment.goplatform-api/src/internal/service/deployment_undeploy_guard_test.goplatform-api/src/internal/service/gateway.goplatform-api/src/internal/service/gateway_internal.goplatform-api/src/internal/service/gateway_properties_test.goplatform-api/src/internal/service/llm.goplatform-api/src/internal/service/llm_deployment.goplatform-api/src/internal/service/mcp.goplatform-api/src/internal/service/mcp_deployment.goplatform-api/src/internal/service/webbroker_api.goplatform-api/src/internal/service/webbroker_api_deployment.goplatform-api/src/internal/service/websub_api.goplatform-api/src/internal/service/websub_api_deployment.goplatform-api/src/internal/utils/api.goplatform-api/src/resources/gateway-internal-api.yamlplatform-api/src/resources/openapi.yaml
💤 Files with no reviewable changes (2)
- platform-api/src/internal/service/gateway_internal.go
- platform-api/src/resources/openapi.yaml
✅ Files skipped from review due to trivial changes (4)
- platform-api/oapi-codegen.yaml
- docs/rest-apis/gateway/README.md
- platform-api/src/internal/handler/webbroker_api_deployment.go
- platform-api/src/api/generated.go
🚧 Files skipped from review as they are similar to previous changes (81)
- platform-api/src/internal/model/artifact.go
- platform-api/src/internal/service/artifact_guard.go
- platform-api/src/internal/handler/webbroker_api.go
- platform-api/src/internal/service/artifact_import_llm_template.go
- gateway/gateway-controller/pkg/models/stored_config.go
- platform-api/src/internal/handler/api.go
- gateway/gateway-controller/pkg/api/handlers/llm_provider_handler.go
- gateway/build-manifest.yaml
- platform-api/src/internal/repository/interfaces.go
- platform-api/src/internal/model/webbroker_api.go
- platform-api/src/internal/model/mcp.go
- platform-api/src/internal/server/server.go
- platform-api/src/internal/service/deployment_undeploy_guard_test.go
- platform-api/src/internal/handler/artifact_guard_response.go
- platform-api/src/internal/service/artifact_import_llm_policies_test.go
- platform-api/src/internal/service/mcp_deployment.go
- platform-api/src/internal/handler/websub_api_deployment.go
- platform-api/src/internal/service/websub_api.go
- gateway/gateway-controller/pkg/utils/api_deployment_test.go
- gateway/gateway-controller/pkg/api/handlers/rest_api_handler.go
- platform-api/src/internal/constants/constants.go
- platform-api/src/internal/utils/api.go
- platform-api/src/internal/handler/gateway.go
- platform-api/src/internal/model/websub_api.go
- gateway/gateway-controller/pkg/api/handlers/handlers_test.go
- platform-api/src/internal/handler/mcp.go
- gateway/gateway-controller/pkg/api/handlers/llm_provider_template_handler.go
- platform-api/src/internal/service/artifact_cross_origin_test.go
- platform-api/src/internal/database/schema.sqlite.sql
- platform-api/src/internal/repository/mcp.go
- platform-api/src/internal/database/schema.sql
- platform-api/src/internal/handler/websub_api.go
- platform-api/src/internal/repository/webbroker_api.go
- platform-api/src/internal/constants/error.go
- platform-api/src/internal/database/schema.postgres.sql
- platform-api/src/internal/service/artifact_import_llm_policies.go
- platform-api/src/internal/service/artifact_readonly_test.go
- platform-api/src/internal/service/webbroker_api_deployment.go
- platform-api/src/internal/handler/api_deployment.go
- platform-api/src/internal/repository/websub_api.go
- platform-api/src/internal/service/artifact_import_llm_provider.go
- gateway/gateway-controller/pkg/api/handlers/mcp_proxy_handler.go
- platform-api/src/internal/service/mcp.go
- platform-api/src/internal/handler/llm.go
- platform-api/src/internal/repository/deployment.go
- platform-api/src/internal/dto/artifact_import.go
- gateway/gateway-controller/pkg/controlplane/push_artifacts_test.go
- platform-api/src/internal/service/gateway_properties_test.go
- platform-api/src/internal/repository/gateway.go
- gateway/gateway-controller/pkg/utils/api_key_test.go
- gateway/gateway-controller/pkg/controlplane/controlplane_test.go
- gateway/gateway-controller/pkg/service/restapi/service.go
- platform-api/src/internal/service/artifact_import_mcp.go
- gateway/gateway-controller/pkg/controlplane/sync.go
- platform-api/src/internal/service/websub_api_deployment.go
- platform-api/src/internal/repository/artifact.go
- platform-api/src/internal/service/webbroker_api.go
- platform-api/src/internal/service/gateway.go
- gateway/gateway-controller/pkg/controlplane/client.go
- gateway/gateway-controller/pkg/utils/api_utils_test.go
- gateway/gateway-controller/pkg/storage/sql_store.go
- platform-api/src/internal/service/artifact_import_llm_proxy.go
- platform-api/src/internal/service/artifact_import_test.go
- platform-api/src/internal/handler/mcp_deployment.go
- platform-api/src/internal/model/llm.go
- platform-api/src/internal/model/gateway.go
- platform-api/src/internal/service/deployment.go
- platform-api/src/internal/model/api.go
- platform-api/src/internal/service/llm_deployment.go
- gateway/gateway-controller/pkg/api/handlers/llm_proxy_handler.go
- platform-api/src/internal/repository/api.go
- platform-api/src/internal/handler/llm_deployment.go
- gateway/gateway-controller/pkg/utils/api_utils.go
- gateway/gateway-controller/pkg/api/handlers/handlers.go
- platform-api/src/internal/service/llm.go
- platform-api/src/internal/dto/gateway_internal.go
- platform-api/src/internal/service/artifact_import.go
- platform-api/src/internal/service/artifact_import_lifecycle_test.go
- platform-api/src/internal/service/api.go
- platform-api/src/internal/repository/llm.go
- platform-api/src/resources/gateway-internal-api.yaml
Comment on lines
+799
to
+803
| ```json | ||
| { | ||
| "status": "success", | ||
| "message": "Webhook secret generated successfully", | ||
| "secret": "whsec_1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b", |
Contributor
There was a problem hiding this comment.
Fix regenerate response message in the example.
For the regenerate endpoint, the sample message on Line 802 should match the actual operation semantics (regenerated, not generated).
Proposed doc fix
- "message": "Webhook secret generated successfully",
+ "message": "Webhook secret regenerated successfully",📝 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.
Suggested change
| ```json | |
| { | |
| "status": "success", | |
| "message": "Webhook secret generated successfully", | |
| "secret": "whsec_1a2b3c4d5e6f7a8b9c0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f5a6b7c8d9e0f1a2b", |
🧰 Tools
🪛 Betterleaks (1.5.0)
[high] 803-803: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 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 `@docs/rest-apis/gateway/websub-api-management.md` around lines 799 - 803, The
example response message for the webhook secret regenerate endpoint is
incorrect. In the JSON response example for the regenerate operation, the
message field currently states "Webhook secret generated successfully" but
should say "Webhook secret regenerated successfully" to accurately reflect that
the endpoint is regenerating an existing secret, not generating a new one for
the first time. Update the message string value in the response example to use
the past tense "regenerated" instead of "generated".
Comment on lines
+858
to
+870
| > 404 Response | ||
|
|
||
| ```json | ||
| { | ||
| "status": "error", | ||
| "message": "Configuration validation failed", | ||
| "errors": [ | ||
| { | ||
| "field": "spec.context", | ||
| "message": "Context must start with / and cannot end with /" | ||
| } | ||
| ] | ||
| } |
Contributor
There was a problem hiding this comment.
Replace the 404 example with a WebSub secret-not-found style error.
The current 404 sample is a configuration-validation payload and does not match this endpoint. Use an example aligned to missing WebSub API or missing secret responses.
Proposed doc fix
{
"status": "error",
- "message": "Configuration validation failed",
- "errors": [
- {
- "field": "spec.context",
- "message": "Context must start with / and cannot end with /"
- }
- ]
+ "message": "Webhook secret 'github-webhook' not found"
}📝 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.
Suggested change
| > 404 Response | |
| ```json | |
| { | |
| "status": "error", | |
| "message": "Configuration validation failed", | |
| "errors": [ | |
| { | |
| "field": "spec.context", | |
| "message": "Context must start with / and cannot end with /" | |
| } | |
| ] | |
| } | |
| > 404 Response | |
🤖 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 `@docs/rest-apis/gateway/websub-api-management.md` around lines 858 - 870, The
404 response example in the WebSub API documentation shows a
configuration-validation error payload which does not match the actual error
responses for this endpoint. Replace the entire 404 response example with a
proper WebSub secret-not-found or WebSub API-not-found style error response that
accurately reflects what would be returned when a WebSub API or secret is
missing or not found on this endpoint.
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
1 participant
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.
Purpose
Related to - #2083
Goals
Approach
Refactor and consolidate the push logic, ensuring that all artifact types use a consistent path for deployment and undeployment notifications. Additionally, it updates several policy versions in the build manifest.
Key changes include:
Unified Artifact Push and Undeploy Mechanism
PushArtifactmethod in the control plane client, replacing the previousPushAPIDeployment. This method now handles pushing any gateway-originated artifact (API, LLM provider, proxy, template) and records the sync status in the database. [1] [2] [3] [4]pushArtifactUndeploylogic to notify the control plane when an artifact is deleted from the gateway, marking it as undeployed but not removing it from the control plane. This is now consistently used across APIs, LLM providers, proxies, and REST APIs. [1] [2] [3] [4] [5] [6] [7]LLM Provider Template Push Support
pushTemplateToControlPlaneto handle pushing LLM provider templates to the control plane on create and update, ensuring templates are always pushed with a "deployed" status. [1] [2] [3] [4]Control Plane Client Improvements
PushGatewayArtifactsToControlPlane, ensuring synchronization after reconnects. [1] [2]REST API Handler Refactor