feat(api-server,sdk,cli): implement Application API with full-stack support#1676
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
📝 WalkthroughWalkthroughThis PR implements a complete Applications resource: OpenAPI contract, server plugin and handler wiring, persistence model/DAO/migrations, service with advisory locks and event emission, RBAC extensions, SDKs (Go/Python/TS), CLI commands, integration tests, and docs updates. ChangesApplications Resource
Possibly related PRs:
Suggested labels: ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
components/ambient-cli/cmd/acpctl/application/cmd.go (1)
1-1: 💤 Low valueAdd package comment for linter compliance.
Static analysis (ST1000) requires at least one file in the package to have a package comment. Add a doc comment above
package applicationdescribing the command group's purpose.🤖 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 `@components/ambient-cli/cmd/acpctl/application/cmd.go` at line 1, Add a package comment above the package declaration for package application describing the purpose of this command group (e.g., "package application provides CLI commands to manage application lifecycle and configuration for acpctl"). Edit the file containing "package application" and insert a short one- or two-sentence doc comment that explains what the application command group does so the ST1000 linter requirement is satisfied.
🤖 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 `@components/ambient-api-server/openapi/openapi.applications.yaml`:
- Around line 365-385: Update the OpenAPI parameter descriptions to reference
the correct resource "applications" instead of "accounts" and correct example
fields/URLs: in the `search` parameter description (the block mentioning
username and subscription_labels) change "accounts" to "applications", use an
applications-specific example such as "name like 'my-app%'" and remove or
replace the subscription_labels example with application-relevant relations; in
the `orderBy` parameter description replace "username" examples with "name" and
update the compound example to "name asc, destination_project asc"; and in the
`fields` parameter example fix the example URL to /api/ambient/v1/applications.
Ensure the parameter blocks named `search`, `orderBy`, and `fields` in
openapi.applications.yaml are updated accordingly.
In `@components/ambient-api-server/plugins/applications/factory_test.go`:
- Around line 12-46: The newApplication function has an unused parameter id;
either use it to initialize the Application (e.g., set application.Name or
another field from id) before calling applicationService.Create, or remove the
id parameter and update all callers such as newApplicationList to stop passing a
value; locate the function newApplication, the caller newApplicationList, and
the creation path via applications.Application and applicationService.Create to
apply the change consistently.
In `@components/ambient-cli/cmd/acpctl/application/cmd.go`:
- Around line 372-374: The create subcommand incorrectly maps the --description
flag to builder.Conditions(createArgs.description) (seen in application/cmd.go
and create/cmd.go's createApplication), but Conditions is a status array; change
the mapping to the correct field: if the Application model exposes Description
use builder.Description(createArgs.description), otherwise remove the Conditions
mapping and instead set an appropriate metadata field such as
builder.Annotations(map[string]string{"ambient.ai/description":
createArgs.description}) or omit the flag mapping entirely; update both
occurrences (builder.Conditions in createApplication and in the CLI command
wiring) to the chosen fix.
---
Nitpick comments:
In `@components/ambient-cli/cmd/acpctl/application/cmd.go`:
- Line 1: Add a package comment above the package declaration for package
application describing the purpose of this command group (e.g., "package
application provides CLI commands to manage application lifecycle and
configuration for acpctl"). Edit the file containing "package application" and
insert a short one- or two-sentence doc comment that explains what the
application command group does so the ST1000 linter requirement is satisfied.
🪄 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: 7ab3ad60-3bd6-4e7f-8697-c9ddd17fe243
⛔ Files ignored due to path filters (11)
components/ambient-api-server/pkg/api/openapi/.openapi-generator/FILESis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/README.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api_default.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Application.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationList.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationPatchRequest.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/DefaultAPI.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_list.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_patch_request.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (108)
components/ambient-api-server/cmd/ambient-api-server/main.gocomponents/ambient-api-server/openapi/openapi.applications.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/hierarchy.gocomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/pkg/rbac/permissions.gocomponents/ambient-api-server/pkg/rbac/scope.gocomponents/ambient-api-server/plugins/applications/dao.gocomponents/ambient-api-server/plugins/applications/factory_test.gocomponents/ambient-api-server/plugins/applications/handler.gocomponents/ambient-api-server/plugins/applications/integration_test.gocomponents/ambient-api-server/plugins/applications/migration.gocomponents/ambient-api-server/plugins/applications/mock_dao.gocomponents/ambient-api-server/plugins/applications/model.gocomponents/ambient-api-server/plugins/applications/plugin.gocomponents/ambient-api-server/plugins/applications/presenter.gocomponents/ambient-api-server/plugins/applications/service.gocomponents/ambient-api-server/plugins/applications/testmain_test.gocomponents/ambient-cli/cmd/acpctl/application/cmd.gocomponents/ambient-cli/cmd/acpctl/create/cmd.gocomponents/ambient-cli/cmd/acpctl/delete/cmd.gocomponents/ambient-cli/cmd/acpctl/describe/cmd.gocomponents/ambient-cli/cmd/acpctl/get/cmd.gocomponents/ambient-cli/cmd/acpctl/main.gocomponents/ambient-sdk/go-sdk/client/agent_api.gocomponents/ambient-sdk/go-sdk/client/application_api.gocomponents/ambient-sdk/go-sdk/client/client.gocomponents/ambient-sdk/go-sdk/client/credential_api.gocomponents/ambient-sdk/go-sdk/client/inbox_message_api.gocomponents/ambient-sdk/go-sdk/client/iterator.gocomponents/ambient-sdk/go-sdk/client/project_api.gocomponents/ambient-sdk/go-sdk/client/project_settings_api.gocomponents/ambient-sdk/go-sdk/client/role_api.gocomponents/ambient-sdk/go-sdk/client/role_binding_api.gocomponents/ambient-sdk/go-sdk/client/scheduled_session_api.gocomponents/ambient-sdk/go-sdk/client/session_api.gocomponents/ambient-sdk/go-sdk/client/session_message_api.gocomponents/ambient-sdk/go-sdk/client/user_api.gocomponents/ambient-sdk/go-sdk/types/agent.gocomponents/ambient-sdk/go-sdk/types/application.gocomponents/ambient-sdk/go-sdk/types/base.gocomponents/ambient-sdk/go-sdk/types/credential.gocomponents/ambient-sdk/go-sdk/types/inbox_message.gocomponents/ambient-sdk/go-sdk/types/list_options.gocomponents/ambient-sdk/go-sdk/types/project.gocomponents/ambient-sdk/go-sdk/types/project_settings.gocomponents/ambient-sdk/go-sdk/types/role.gocomponents/ambient-sdk/go-sdk/types/role_binding.gocomponents/ambient-sdk/go-sdk/types/scheduled_session.gocomponents/ambient-sdk/go-sdk/types/session.gocomponents/ambient-sdk/go-sdk/types/session_message.gocomponents/ambient-sdk/go-sdk/types/user.gocomponents/ambient-sdk/python-sdk/ambient_platform/__init__.pycomponents/ambient-sdk/python-sdk/ambient_platform/_agent_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_application_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_base.pycomponents/ambient-sdk/python-sdk/ambient_platform/_credential_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_inbox_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_iterator.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_settings_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_binding_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_scheduled_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_user_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/agent.pycomponents/ambient-sdk/python-sdk/ambient_platform/application.pycomponents/ambient-sdk/python-sdk/ambient_platform/client.pycomponents/ambient-sdk/python-sdk/ambient_platform/credential.pycomponents/ambient-sdk/python-sdk/ambient_platform/inbox_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/project.pycomponents/ambient-sdk/python-sdk/ambient_platform/project_settings.pycomponents/ambient-sdk/python-sdk/ambient_platform/role.pycomponents/ambient-sdk/python-sdk/ambient_platform/role_binding.pycomponents/ambient-sdk/python-sdk/ambient_platform/scheduled_session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/user.pycomponents/ambient-sdk/ts-sdk/src/agent.tscomponents/ambient-sdk/ts-sdk/src/agent_api.tscomponents/ambient-sdk/ts-sdk/src/application.tscomponents/ambient-sdk/ts-sdk/src/application_api.tscomponents/ambient-sdk/ts-sdk/src/base.tscomponents/ambient-sdk/ts-sdk/src/client.tscomponents/ambient-sdk/ts-sdk/src/credential.tscomponents/ambient-sdk/ts-sdk/src/credential_api.tscomponents/ambient-sdk/ts-sdk/src/inbox_message.tscomponents/ambient-sdk/ts-sdk/src/inbox_message_api.tscomponents/ambient-sdk/ts-sdk/src/index.tscomponents/ambient-sdk/ts-sdk/src/project.tscomponents/ambient-sdk/ts-sdk/src/project_api.tscomponents/ambient-sdk/ts-sdk/src/project_settings.tscomponents/ambient-sdk/ts-sdk/src/project_settings_api.tscomponents/ambient-sdk/ts-sdk/src/role.tscomponents/ambient-sdk/ts-sdk/src/role_api.tscomponents/ambient-sdk/ts-sdk/src/role_binding.tscomponents/ambient-sdk/ts-sdk/src/role_binding_api.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session_api.tscomponents/ambient-sdk/ts-sdk/src/session.tscomponents/ambient-sdk/ts-sdk/src/session_api.tscomponents/ambient-sdk/ts-sdk/src/session_message.tscomponents/ambient-sdk/ts-sdk/src/session_message_api.tscomponents/ambient-sdk/ts-sdk/src/user.tscomponents/ambient-sdk/ts-sdk/src/user_api.tsworkflows/sessions/ambient-model.workflow.md
| description: |- | ||
| Specifies the search criteria. The syntax of this parameter is | ||
| similar to the syntax of the _where_ clause of an SQL statement, | ||
| using the names of the json attributes / column names of the account. | ||
| For example, in order to retrieve all the accounts with a username | ||
| starting with `my`: | ||
|
|
||
| ```sql | ||
| username like 'my%' | ||
| ``` | ||
|
|
||
| The search criteria can also be applied on related resource. | ||
| For example, in order to retrieve all the subscriptions labeled by `foo=bar`, | ||
|
|
||
| ```sql | ||
| subscription_labels.key = 'foo' and subscription_labels.value = 'bar' | ||
| ``` | ||
|
|
||
| If the parameter isn't provided, or if the value is empty, then | ||
| all the accounts that the user has permission to see will be | ||
| returned. |
There was a problem hiding this comment.
Fix copy-paste documentation errors in parameter descriptions.
Three parameter descriptions reference the wrong resource types:
- Line 365-385:
searchparameter description mentions "accounts" and "subscription_labels" instead of "applications" and application-related fields - Line 392-410:
orderByparameter description uses "accounts" examples instead of "applications" - Line 423:
fieldsparameter example URL references/api/ambient/v1/sessionsinstead of/api/ambient/v1/applications
These are copy-pasted from other resource specs and will confuse API consumers.
📝 Suggested fixes
For the search parameter (lines 365-385), replace "accounts" with "applications" and update the example:
- starting with `my`:
+ starting with `my-app`:
```sql
- username like 'my%'
+ name like 'my-app%'
```
- The search criteria can also be applied on related resource.
- For example, in order to retrieve all the subscriptions labeled by `foo=bar`,
-
- ```sql
- subscription_labels.key = 'foo' and subscription_labels.value = 'bar'
- ```
-
If the parameter isn't provided, or if the value is empty, then
- all the accounts that the user has permission to see will be
+ all the applications that the user has permission to see will be
returned.For the orderBy parameter (lines 392-410), replace "accounts" with "applications":
similar to the syntax of the _order by_ clause of an SQL statement,
- but using the names of the json attributes / column of the account.
- For example, in order to retrieve all accounts ordered by username:
+ but using the names of the json attributes / column of the application.
+ For example, in order to retrieve all applications ordered by name:
```sql
- username asc
+ name asc
```
- Or in order to retrieve all accounts ordered by username _and_ first name:
+ Or in order to retrieve all applications ordered by name _and_ destination project:
```sql
- username asc, firstName asc
+ name asc, destination_project asc
```For the fields parameter (line 423), fix the example URL:
- curl "/api/ambient/v1/sessions?fields=id,href,name,project_id"
+ curl "/api/ambient/v1/applications?fields=id,href,name,destination_project"Also applies to: 392-410, 423-423
🤖 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 `@components/ambient-api-server/openapi/openapi.applications.yaml` around lines
365 - 385, Update the OpenAPI parameter descriptions to reference the correct
resource "applications" instead of "accounts" and correct example fields/URLs:
in the `search` parameter description (the block mentioning username and
subscription_labels) change "accounts" to "applications", use an
applications-specific example such as "name like 'my-app%'" and remove or
replace the subscription_labels example with application-relevant relations; in
the `orderBy` parameter description replace "username" examples with "name" and
update the compound example to "name asc, destination_project asc"; and in the
`fields` parameter example fix the example URL to /api/ambient/v1/applications.
Ensure the parameter blocks named `search`, `orderBy`, and `fields` in
openapi.applications.yaml are updated accordingly.
| func newApplication(id string) (*applications.Application, error) { | ||
| applicationService := applications.Service(&environments.Environment().Services) | ||
|
|
||
| application := &applications.Application{ | ||
| Name: "test-name", | ||
| SourceRepoUrl: "test-source_repo_url", | ||
| SourceTargetRevision: stringPtr("test-source_target_revision"), | ||
| SourcePath: "test-source_path", | ||
| DestinationAmbientUrl: stringPtr("test-destination_ambient_url"), | ||
| DestinationProject: "test-destination_project", | ||
| CredentialId: stringPtr("test-credential_id"), | ||
| AutoSync: boolPtr(true), | ||
| AutoPrune: boolPtr(true), | ||
| SelfHeal: boolPtr(true), | ||
| SyncOptions: stringPtr("test-sync_options"), | ||
| RetryLimit: intPtr(42), | ||
| SyncStatus: stringPtr("test-sync_status"), | ||
| HealthStatus: stringPtr("test-health_status"), | ||
| SyncRevision: stringPtr("test-sync_revision"), | ||
| OperationPhase: stringPtr("test-operation_phase"), | ||
| OperationMessage: stringPtr("test-operation_message"), | ||
| ResourceStatus: stringPtr("test-resource_status"), | ||
| Conditions: stringPtr("test-conditions"), | ||
| Labels: stringPtr("test-labels"), | ||
| Annotations: stringPtr("test-annotations"), | ||
| LastSyncedAt: timePtr(time.Now()), | ||
| } | ||
|
|
||
| sub, err := applicationService.Create(context.Background(), application) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return sub, nil | ||
| } |
There was a problem hiding this comment.
Unused id parameter in newApplication function.
The function signature includes id string but never uses it. The service layer assigns a new ID via api.NewID() in the BeforeCreate hook, rendering the parameter pointless. At line 51, newApplicationList passes a constructed name to this parameter, but that value is also discarded.
🔧 Suggested fix
If the parameter is meant to seed the application name for list generation, use it:
func newApplication(id string) (*applications.Application, error) {
applicationService := applications.Service(&environments.Environment().Services)
application := &applications.Application{
- Name: "test-name",
+ Name: id,
SourceRepoUrl: "test-source_repo_url",Otherwise, remove the unused parameter:
-func newApplication(id string) (*applications.Application, error) {
+func newApplication() (*applications.Application, error) {
applicationService := applications.Service(&environments.Environment().Services)
application := &applications.Application{And update the caller:
for i := 1; i <= count; i++ {
name := fmt.Sprintf("%s_%d", namePrefix, i)
- c, err := newApplication(name)
+ c, err := newApplication()📝 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.
| func newApplication(id string) (*applications.Application, error) { | |
| applicationService := applications.Service(&environments.Environment().Services) | |
| application := &applications.Application{ | |
| Name: "test-name", | |
| SourceRepoUrl: "test-source_repo_url", | |
| SourceTargetRevision: stringPtr("test-source_target_revision"), | |
| SourcePath: "test-source_path", | |
| DestinationAmbientUrl: stringPtr("test-destination_ambient_url"), | |
| DestinationProject: "test-destination_project", | |
| CredentialId: stringPtr("test-credential_id"), | |
| AutoSync: boolPtr(true), | |
| AutoPrune: boolPtr(true), | |
| SelfHeal: boolPtr(true), | |
| SyncOptions: stringPtr("test-sync_options"), | |
| RetryLimit: intPtr(42), | |
| SyncStatus: stringPtr("test-sync_status"), | |
| HealthStatus: stringPtr("test-health_status"), | |
| SyncRevision: stringPtr("test-sync_revision"), | |
| OperationPhase: stringPtr("test-operation_phase"), | |
| OperationMessage: stringPtr("test-operation_message"), | |
| ResourceStatus: stringPtr("test-resource_status"), | |
| Conditions: stringPtr("test-conditions"), | |
| Labels: stringPtr("test-labels"), | |
| Annotations: stringPtr("test-annotations"), | |
| LastSyncedAt: timePtr(time.Now()), | |
| } | |
| sub, err := applicationService.Create(context.Background(), application) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return sub, nil | |
| } | |
| func newApplication(id string) (*applications.Application, error) { | |
| applicationService := applications.Service(&environments.Environment().Services) | |
| application := &applications.Application{ | |
| Name: id, | |
| SourceRepoUrl: "test-source_repo_url", | |
| SourceTargetRevision: stringPtr("test-source_target_revision"), | |
| SourcePath: "test-source_path", | |
| DestinationAmbientUrl: stringPtr("test-destination_ambient_url"), | |
| DestinationProject: "test-destination_project", | |
| CredentialId: stringPtr("test-credential_id"), | |
| AutoSync: boolPtr(true), | |
| AutoPrune: boolPtr(true), | |
| SelfHeal: boolPtr(true), | |
| SyncOptions: stringPtr("test-sync_options"), | |
| RetryLimit: intPtr(42), | |
| SyncStatus: stringPtr("test-sync_status"), | |
| HealthStatus: stringPtr("test-health_status"), | |
| SyncRevision: stringPtr("test-sync_revision"), | |
| OperationPhase: stringPtr("test-operation_phase"), | |
| OperationMessage: stringPtr("test-operation_message"), | |
| ResourceStatus: stringPtr("test-resource_status"), | |
| Conditions: stringPtr("test-conditions"), | |
| Labels: stringPtr("test-labels"), | |
| Annotations: stringPtr("test-annotations"), | |
| LastSyncedAt: timePtr(time.Now()), | |
| } | |
| sub, err := applicationService.Create(context.Background(), application) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return sub, nil | |
| } |
🤖 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 `@components/ambient-api-server/plugins/applications/factory_test.go` around
lines 12 - 46, The newApplication function has an unused parameter id; either
use it to initialize the Application (e.g., set application.Name or another
field from id) before calling applicationService.Create, or remove the id
parameter and update all callers such as newApplicationList to stop passing a
value; locate the function newApplication, the caller newApplicationList, and
the creation path via applications.Application and applicationService.Create to
apply the change consistently.
321ed56 to
71d4bf9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/ambient-api-server/plugins/applications/handler.go (1)
214-233: 💤 Low valueSync and Refresh use
HandleGetdespite being POST mutations.Both
Sync(line 232) andRefresh(line 253) callhandlers.HandleGeteven though they mutate state viaReplace. This works functionally but is semantically misleading—HandleGettypically implies read-only operations and may have different middleware/logging behavior.Consider using
handlers.Handle(w, r, cfg, http.StatusOK)likePatchdoes for consistency.Also applies to: 235-254
🤖 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 `@components/ambient-api-server/plugins/applications/handler.go` around lines 214 - 233, Sync and Refresh are implemented as mutations (they call h.application.Replace and change app.OperationPhase) but use handlers.HandleGet, which is semantically for read-only ops; update both Sync and Refresh to call handlers.Handle(w, r, cfg, http.StatusOK) (same pattern as Patch) so middleware/logging and intent match a POST mutation—locate the Sync and Refresh methods in applicationHandler and replace the handlers.HandleGet call with handlers.Handle(w, r, cfg, http.StatusOK) while keeping the existing handlers.HandlerConfig and return values intact.components/ambient-cli/cmd/acpctl/session/messages.go (1)
27-27: ⚡ Quick winDead code: underscore assignments serve no purpose.
The four underscore assignments with "reserved" comments create lipgloss values and immediately discard them. They don't reserve colors or styles for future use — they're just dead code. Consider removing lines 27, 30, 37, and 40 entirely.
♻️ Remove dead code
sseColorDim = lipgloss.Color("240") sseColorCyan = lipgloss.Color("36") - _ = lipgloss.Color("69") // sseColorBlue reserved for agent message rendering sseColorGreen = lipgloss.Color("28") sseColorRed = lipgloss.Color("196") - _ = lipgloss.Color("214") // sseColorYellow reserved for agent message rendering sseColorMagenta = lipgloss.Color("134") sseThinkTag = lipgloss.NewStyle().Foreground(sseColorDim).Bold(true) sseThinkText = lipgloss.NewStyle().Foreground(sseColorDim).Italic(true) sseToolTag = lipgloss.NewStyle().Foreground(sseColorCyan).Bold(true) sseToolResult = lipgloss.NewStyle().Foreground(sseColorDim) - _ = lipgloss.NewStyle().Foreground(lipgloss.Color("255")) // sseAssistant reserved for agent message rendering sseRunTag = lipgloss.NewStyle().Foreground(sseColorGreen).Bold(true) sseErrorTag = lipgloss.NewStyle().Foreground(sseColorRed).Bold(true) - _ = lipgloss.NewStyle().Foreground(lipgloss.Color("214")).Bold(true) // sseAgentTag reserved for agent message rendering sseStepTag = lipgloss.NewStyle().Foreground(sseColorMagenta).Bold(true)Also applies to: 30-30, 37-37, 40-40
🤖 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 `@components/ambient-cli/cmd/acpctl/session/messages.go` at line 27, Remove the four dead underscore assignments that call lipgloss.Color (the lines like `_ = lipgloss.Color("69") // sseColorBlue reserved for agent message rendering`) — delete those no-op statements; if the intent was to reserve named styles/colors instead of discarding them, replace each with a proper named declaration (e.g., const or var with a clear identifier) rather than an underscore assignment; locate the occurrences by searching for `lipgloss.Color("` in messages.go and update accordingly.
🤖 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 `@components/ambient-api-server/plugins/applications/service.go`:
- Around line 131-134: The Delete method in sqlApplicationService wraps the DAO
error with errors.GeneralError before calling services.HandleDeleteError, which
hides original error types; change Delete to pass the raw error returned from
applicationDao.Delete(ctx, id) directly into services.HandleDeleteError (same
pattern used by Create/Replace) so HandleDeleteError can inspect the original
error (e.g., NotFound) and return the correct status; locate the Delete
function, the applicationDao.Delete call, and replace the errors.GeneralError
wrapping with the original err when calling services.HandleDeleteError.
- Around line 156-162: The All method on sqlApplicationService returns all
Application rows unbounded (function All calling s.applicationDao.All) which can
OOM; update the API to enforce pagination by either (A) replacing/marking All as
deprecated and routing callers to the existing paginated List(ctx,
pageToken/limit) method, or (B) change All signature to accept explicit
limit/offset or a ListOptions struct and pass that limit through to
applicationDao (or add a new AllWithLimit/ListAllLimited wrapper), and update
all call sites to use the paginated/ListOptions variant so no code path fetches
unbounded rows.
- Around line 76-92: The event creation after persisting the Application can
fail and leave orphaned state; in sqlApplicationService.Create (and similarly in
Update/Delete) you must ensure atomicity by performing a compensating rollback
when s.events.Create returns an error: after s.applicationDao.Create succeeds,
if s.events.Create(ctx, ...) returns evErr then call the appropriate DAO
compensation (e.g., s.applicationDao.Delete(ctx, application.ID) for Create, or
restore the previous record for Update, or re-insert the deleted record for
Delete), handle/aggregate errors, and then return a ServiceError (don't silently
succeed); alternatively, if the event store supports transactions, wrap
s.applicationDao.* and s.events.Create in a transaction instead — update the
Create/Update/Delete methods to use the rollback or transaction approach and
reference s.applicationDao.Create / s.applicationDao.Update /
s.applicationDao.Delete and s.events.Create when locating the changes.
---
Nitpick comments:
In `@components/ambient-api-server/plugins/applications/handler.go`:
- Around line 214-233: Sync and Refresh are implemented as mutations (they call
h.application.Replace and change app.OperationPhase) but use handlers.HandleGet,
which is semantically for read-only ops; update both Sync and Refresh to call
handlers.Handle(w, r, cfg, http.StatusOK) (same pattern as Patch) so
middleware/logging and intent match a POST mutation—locate the Sync and Refresh
methods in applicationHandler and replace the handlers.HandleGet call with
handlers.Handle(w, r, cfg, http.StatusOK) while keeping the existing
handlers.HandlerConfig and return values intact.
In `@components/ambient-cli/cmd/acpctl/session/messages.go`:
- Line 27: Remove the four dead underscore assignments that call lipgloss.Color
(the lines like `_ = lipgloss.Color("69") // sseColorBlue reserved for agent
message rendering`) — delete those no-op statements; if the intent was to
reserve named styles/colors instead of discarding them, replace each with a
proper named declaration (e.g., const or var with a clear identifier) rather
than an underscore assignment; locate the occurrences by searching for
`lipgloss.Color("` in messages.go and update accordingly.
🪄 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: ac10a3db-263b-40ce-a00f-cc25fcf50c3a
⛔ Files ignored due to path filters (11)
components/ambient-api-server/pkg/api/openapi/.openapi-generator/FILESis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/README.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api_default.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Application.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationList.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationPatchRequest.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/DefaultAPI.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_list.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_patch_request.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (109)
components/ambient-api-server/cmd/ambient-api-server/main.gocomponents/ambient-api-server/openapi/openapi.applications.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/hierarchy.gocomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/pkg/rbac/permissions.gocomponents/ambient-api-server/pkg/rbac/scope.gocomponents/ambient-api-server/plugins/applications/dao.gocomponents/ambient-api-server/plugins/applications/factory_test.gocomponents/ambient-api-server/plugins/applications/handler.gocomponents/ambient-api-server/plugins/applications/integration_test.gocomponents/ambient-api-server/plugins/applications/migration.gocomponents/ambient-api-server/plugins/applications/mock_dao.gocomponents/ambient-api-server/plugins/applications/model.gocomponents/ambient-api-server/plugins/applications/plugin.gocomponents/ambient-api-server/plugins/applications/presenter.gocomponents/ambient-api-server/plugins/applications/service.gocomponents/ambient-api-server/plugins/applications/testmain_test.gocomponents/ambient-cli/cmd/acpctl/application/cmd.gocomponents/ambient-cli/cmd/acpctl/create/cmd.gocomponents/ambient-cli/cmd/acpctl/delete/cmd.gocomponents/ambient-cli/cmd/acpctl/describe/cmd.gocomponents/ambient-cli/cmd/acpctl/get/cmd.gocomponents/ambient-cli/cmd/acpctl/main.gocomponents/ambient-cli/cmd/acpctl/session/messages.gocomponents/ambient-sdk/go-sdk/client/agent_api.gocomponents/ambient-sdk/go-sdk/client/application_api.gocomponents/ambient-sdk/go-sdk/client/client.gocomponents/ambient-sdk/go-sdk/client/credential_api.gocomponents/ambient-sdk/go-sdk/client/inbox_message_api.gocomponents/ambient-sdk/go-sdk/client/iterator.gocomponents/ambient-sdk/go-sdk/client/project_api.gocomponents/ambient-sdk/go-sdk/client/project_settings_api.gocomponents/ambient-sdk/go-sdk/client/role_api.gocomponents/ambient-sdk/go-sdk/client/role_binding_api.gocomponents/ambient-sdk/go-sdk/client/scheduled_session_api.gocomponents/ambient-sdk/go-sdk/client/session_api.gocomponents/ambient-sdk/go-sdk/client/session_message_api.gocomponents/ambient-sdk/go-sdk/client/user_api.gocomponents/ambient-sdk/go-sdk/types/agent.gocomponents/ambient-sdk/go-sdk/types/application.gocomponents/ambient-sdk/go-sdk/types/base.gocomponents/ambient-sdk/go-sdk/types/credential.gocomponents/ambient-sdk/go-sdk/types/inbox_message.gocomponents/ambient-sdk/go-sdk/types/list_options.gocomponents/ambient-sdk/go-sdk/types/project.gocomponents/ambient-sdk/go-sdk/types/project_settings.gocomponents/ambient-sdk/go-sdk/types/role.gocomponents/ambient-sdk/go-sdk/types/role_binding.gocomponents/ambient-sdk/go-sdk/types/scheduled_session.gocomponents/ambient-sdk/go-sdk/types/session.gocomponents/ambient-sdk/go-sdk/types/session_message.gocomponents/ambient-sdk/go-sdk/types/user.gocomponents/ambient-sdk/python-sdk/ambient_platform/__init__.pycomponents/ambient-sdk/python-sdk/ambient_platform/_agent_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_application_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_base.pycomponents/ambient-sdk/python-sdk/ambient_platform/_credential_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_inbox_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_iterator.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_settings_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_binding_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_scheduled_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_user_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/agent.pycomponents/ambient-sdk/python-sdk/ambient_platform/application.pycomponents/ambient-sdk/python-sdk/ambient_platform/client.pycomponents/ambient-sdk/python-sdk/ambient_platform/credential.pycomponents/ambient-sdk/python-sdk/ambient_platform/inbox_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/project.pycomponents/ambient-sdk/python-sdk/ambient_platform/project_settings.pycomponents/ambient-sdk/python-sdk/ambient_platform/role.pycomponents/ambient-sdk/python-sdk/ambient_platform/role_binding.pycomponents/ambient-sdk/python-sdk/ambient_platform/scheduled_session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/user.pycomponents/ambient-sdk/ts-sdk/src/agent.tscomponents/ambient-sdk/ts-sdk/src/agent_api.tscomponents/ambient-sdk/ts-sdk/src/application.tscomponents/ambient-sdk/ts-sdk/src/application_api.tscomponents/ambient-sdk/ts-sdk/src/base.tscomponents/ambient-sdk/ts-sdk/src/client.tscomponents/ambient-sdk/ts-sdk/src/credential.tscomponents/ambient-sdk/ts-sdk/src/credential_api.tscomponents/ambient-sdk/ts-sdk/src/inbox_message.tscomponents/ambient-sdk/ts-sdk/src/inbox_message_api.tscomponents/ambient-sdk/ts-sdk/src/index.tscomponents/ambient-sdk/ts-sdk/src/project.tscomponents/ambient-sdk/ts-sdk/src/project_api.tscomponents/ambient-sdk/ts-sdk/src/project_settings.tscomponents/ambient-sdk/ts-sdk/src/project_settings_api.tscomponents/ambient-sdk/ts-sdk/src/role.tscomponents/ambient-sdk/ts-sdk/src/role_api.tscomponents/ambient-sdk/ts-sdk/src/role_binding.tscomponents/ambient-sdk/ts-sdk/src/role_binding_api.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session_api.tscomponents/ambient-sdk/ts-sdk/src/session.tscomponents/ambient-sdk/ts-sdk/src/session_api.tscomponents/ambient-sdk/ts-sdk/src/session_message.tscomponents/ambient-sdk/ts-sdk/src/session_message_api.tscomponents/ambient-sdk/ts-sdk/src/user.tscomponents/ambient-sdk/ts-sdk/src/user_api.tsworkflows/sessions/ambient-model.workflow.md
✅ Files skipped from review due to trivial changes (76)
- components/ambient-sdk/go-sdk/types/credential.go
- components/ambient-sdk/python-sdk/ambient_platform/project_settings.py
- components/ambient-sdk/python-sdk/ambient_platform/_project_api.py
- components/ambient-sdk/python-sdk/ambient_platform/_user_api.py
- components/ambient-sdk/python-sdk/ambient_platform/inbox_message.py
- components/ambient-sdk/go-sdk/client/project_settings_api.go
- components/ambient-sdk/go-sdk/client/credential_api.go
- components/ambient-sdk/ts-sdk/src/session.ts
- components/ambient-sdk/python-sdk/ambient_platform/role_binding.py
- components/ambient-sdk/go-sdk/types/role.go
- components/ambient-sdk/go-sdk/types/project.go
- components/ambient-sdk/go-sdk/client/user_api.go
- components/ambient-sdk/ts-sdk/src/role_binding.ts
- components/ambient-sdk/ts-sdk/src/agent.ts
- components/ambient-sdk/go-sdk/client/project_api.go
- components/ambient-sdk/go-sdk/client/client.go
- components/ambient-sdk/ts-sdk/src/session_api.ts
- components/ambient-sdk/go-sdk/client/agent_api.go
- components/ambient-api-server/pkg/rbac/scope.go
- components/ambient-sdk/python-sdk/ambient_platform/user.py
- components/ambient-sdk/ts-sdk/src/scheduled_session_api.ts
- components/ambient-sdk/go-sdk/client/session_api.go
- components/ambient-sdk/ts-sdk/src/credential.ts
- components/ambient-sdk/go-sdk/types/project_settings.go
- components/ambient-sdk/ts-sdk/src/role_binding_api.ts
- components/ambient-sdk/python-sdk/ambient_platform/_project_settings_api.py
- components/ambient-sdk/ts-sdk/src/project_settings_api.ts
- components/ambient-sdk/python-sdk/ambient_platform/_credential_api.py
- components/ambient-sdk/ts-sdk/src/user.ts
- components/ambient-sdk/ts-sdk/src/role.ts
- components/ambient-sdk/go-sdk/client/role_api.go
- components/ambient-sdk/python-sdk/ambient_platform/_iterator.py
- components/ambient-sdk/go-sdk/client/role_binding_api.go
- components/ambient-sdk/go-sdk/client/iterator.go
- components/ambient-sdk/go-sdk/types/user.go
- components/ambient-sdk/python-sdk/ambient_platform/_role_binding_api.py
- components/ambient-sdk/go-sdk/client/scheduled_session_api.go
- components/ambient-sdk/python-sdk/ambient_platform/credential.py
- components/ambient-sdk/ts-sdk/src/base.ts
- components/ambient-sdk/go-sdk/types/inbox_message.go
- components/ambient-sdk/python-sdk/ambient_platform/_agent_api.py
- components/ambient-sdk/python-sdk/ambient_platform/_role_api.py
- components/ambient-sdk/ts-sdk/src/project_settings.ts
- components/ambient-sdk/go-sdk/types/session_message.go
- components/ambient-sdk/python-sdk/ambient_platform/role.py
- components/ambient-sdk/ts-sdk/src/session_message_api.ts
- components/ambient-sdk/ts-sdk/src/scheduled_session.ts
- components/ambient-sdk/python-sdk/ambient_platform/_session_api.py
- components/ambient-sdk/ts-sdk/src/inbox_message.ts
- components/ambient-sdk/ts-sdk/src/project.ts
- components/ambient-sdk/go-sdk/types/list_options.go
- components/ambient-sdk/ts-sdk/src/credential_api.ts
- components/ambient-sdk/go-sdk/types/base.go
- components/ambient-sdk/python-sdk/ambient_platform/_inbox_message_api.py
- components/ambient-sdk/python-sdk/ambient_platform/session.py
- components/ambient-sdk/python-sdk/ambient_platform/session_message.py
- components/ambient-api-server/pkg/rbac/hierarchy.go
- components/ambient-sdk/go-sdk/types/scheduled_session.go
- components/ambient-sdk/python-sdk/ambient_platform/scheduled_session.py
- components/ambient-sdk/ts-sdk/src/project_api.ts
- components/ambient-sdk/ts-sdk/src/application_api.ts
- components/ambient-sdk/ts-sdk/src/index.ts
- components/ambient-sdk/ts-sdk/src/session_message.ts
- components/ambient-sdk/ts-sdk/src/user_api.ts
- components/ambient-sdk/ts-sdk/src/agent_api.ts
- components/ambient-sdk/go-sdk/types/role_binding.go
- components/ambient-sdk/python-sdk/ambient_platform/project.py
- components/ambient-sdk/python-sdk/ambient_platform/agent.py
- workflows/sessions/ambient-model.workflow.md
- components/ambient-sdk/python-sdk/ambient_platform/_application_api.py
- components/ambient-sdk/go-sdk/types/application.go
- components/ambient-sdk/go-sdk/types/agent.go
- components/ambient-sdk/ts-sdk/src/application.ts
- components/ambient-sdk/python-sdk/ambient_platform/application.py
- components/ambient-sdk/go-sdk/client/application_api.go
- components/ambient-sdk/python-sdk/ambient_platform/_session_message_api.py
🚧 Files skipped from review as they are similar to previous changes (27)
- components/ambient-sdk/go-sdk/types/session.go
- components/ambient-sdk/python-sdk/ambient_platform/_scheduled_session_api.py
- components/ambient-sdk/python-sdk/ambient_platform/client.py
- components/ambient-sdk/ts-sdk/src/role_api.ts
- components/ambient-sdk/ts-sdk/src/inbox_message_api.ts
- components/ambient-api-server/plugins/applications/testmain_test.go
- components/ambient-cli/cmd/acpctl/delete/cmd.go
- components/ambient-sdk/ts-sdk/src/client.ts
- components/ambient-api-server/cmd/ambient-api-server/main.go
- components/ambient-cli/cmd/acpctl/main.go
- components/ambient-api-server/pkg/rbac/middleware.go
- components/ambient-api-server/plugins/applications/mock_dao.go
- components/ambient-sdk/python-sdk/ambient_platform/init.py
- components/ambient-api-server/openapi/openapi.yaml
- components/ambient-api-server/pkg/rbac/permissions.go
- components/ambient-api-server/plugins/applications/model.go
- components/ambient-api-server/plugins/applications/migration.go
- components/ambient-api-server/plugins/applications/presenter.go
- components/ambient-cli/cmd/acpctl/get/cmd.go
- components/ambient-api-server/plugins/applications/factory_test.go
- components/ambient-api-server/plugins/applications/dao.go
- components/ambient-cli/cmd/acpctl/create/cmd.go
- components/ambient-cli/cmd/acpctl/application/cmd.go
- components/ambient-api-server/plugins/applications/plugin.go
- components/ambient-cli/cmd/acpctl/describe/cmd.go
- components/ambient-api-server/openapi/openapi.applications.yaml
- components/ambient-api-server/plugins/applications/integration_test.go
| func (s *sqlApplicationService) Delete(ctx context.Context, id string) *errors.ServiceError { | ||
| if err := s.applicationDao.Delete(ctx, id); err != nil { | ||
| return services.HandleDeleteError("Application", errors.GeneralError("Unable to delete application: %s", err)) | ||
| } |
There was a problem hiding this comment.
Inconsistent error wrapping in Delete.
Delete wraps the DAO error in errors.GeneralError before passing to HandleDeleteError:
return services.HandleDeleteError("Application", errors.GeneralError("Unable to delete application: %s", err))Create and Replace pass the raw error directly to HandleCreateError/HandleUpdateError. This inconsistency may mask the original error type (e.g., NotFound) and return incorrect HTTP status codes.
Proposed fix
func (s *sqlApplicationService) Delete(ctx context.Context, id string) *errors.ServiceError {
if err := s.applicationDao.Delete(ctx, id); err != nil {
- return services.HandleDeleteError("Application", errors.GeneralError("Unable to delete application: %s", err))
+ return services.HandleDeleteError("Application", 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 `@components/ambient-api-server/plugins/applications/service.go` around lines
131 - 134, The Delete method in sqlApplicationService wraps the DAO error with
errors.GeneralError before calling services.HandleDeleteError, which hides
original error types; change Delete to pass the raw error returned from
applicationDao.Delete(ctx, id) directly into services.HandleDeleteError (same
pattern used by Create/Replace) so HandleDeleteError can inspect the original
error (e.g., NotFound) and return the correct status; locate the Delete
function, the applicationDao.Delete call, and replace the errors.GeneralError
wrapping with the original err when calling services.HandleDeleteError.
| func (s *sqlApplicationService) All(ctx context.Context) (ApplicationList, *errors.ServiceError) { | ||
| applications, err := s.applicationDao.All(ctx) | ||
| if err != nil { | ||
| return nil, errors.GeneralError("Unable to get all applications: %s", err) | ||
| } | ||
| return applications, nil | ||
| } |
There was a problem hiding this comment.
All() returns unbounded results without pagination.
Per coding guidelines: "Flag missing pagination/limits on Kubernetes List operations or API endpoints." All() fetches every Application row with no limit. If this is exposed to callers or used in batch processing, it risks OOM on large datasets.
Consider adding a limit parameter or deprecating in favor of paginated List.
🤖 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 `@components/ambient-api-server/plugins/applications/service.go` around lines
156 - 162, The All method on sqlApplicationService returns all Application rows
unbounded (function All calling s.applicationDao.All) which can OOM; update the
API to enforce pagination by either (A) replacing/marking All as deprecated and
routing callers to the existing paginated List(ctx, pageToken/limit) method, or
(B) change All signature to accept explicit limit/offset or a ListOptions struct
and pass that limit through to applicationDao (or add a new
AllWithLimit/ListAllLimited wrapper), and update all call sites to use the
paginated/ListOptions variant so no code path fetches unbounded rows.
Source: Coding guidelines
71d4bf9 to
92f4256
Compare
…upport Add the Application kind (GitOps continuous sync) across the API server, SDK (Go/Python/TS), CLI, and RBAC layers. The Application resource binds a git repository to an Ambient project for continuous agent fleet sync. API Server: - OpenAPI spec with full CRUD + sync/refresh/status sub-resources - Plugin with model, DAO, service, handler, presenter, migration - Integration tests (Get, Post, Patch, Paging, ListSearch) - RBAC: gitops:admin and gitops:viewer roles with permission seeds SDK: - Go/Python/TS clients generated via make generate-sdk - ApplicationBuilder and ApplicationPatchBuilder with validation CLI: - Standalone `acpctl application` command (list/get/create/update/delete/sync/refresh) - Generic commands updated: get, create, delete, describe all support applications - Aliases: application, app, apps 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
92f4256 to
90378f5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@components/ambient-api-server/plugins/applications/mock_dao.go`:
- Around line 35-45: The mock DAO currently returns NotImplemented for Replace,
Delete, and FindByIDs; implement stateful in-memory behavior on
applicationDaoMock by adding/using an internal map[string]*Application (or
map[string]Application) and appropriate locking if concurrency is possible;
implement Replace(ctx, application *Application) to upsert the application into
the map and return the stored copy, Delete(ctx, id string) to remove the entry
and return nil or a not-found error consistent with other mocks, and
FindByIDs(ctx, ids []string) to gather and return an ApplicationList in the same
order as ids (omitting or returning placeholders for missing entries per mock
convention); update applicationDaoMock methods Replace, Delete, and FindByIDs to
use that map and ensure results match expected types.
🪄 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: 17ee651f-129e-4287-aeb5-3d59805ea6c6
⛔ Files ignored due to path filters (11)
components/ambient-api-server/pkg/api/openapi/.openapi-generator/FILESis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/README.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api/openapi.yamlis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/api_default.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/Application.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationList.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/ApplicationPatchRequest.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/docs/DefaultAPI.mdis excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_list.gois excluded by!**/pkg/api/openapi/**components/ambient-api-server/pkg/api/openapi/model_application_patch_request.gois excluded by!**/pkg/api/openapi/**
📒 Files selected for processing (109)
components/ambient-api-server/cmd/ambient-api-server/main.gocomponents/ambient-api-server/openapi/openapi.applications.yamlcomponents/ambient-api-server/openapi/openapi.yamlcomponents/ambient-api-server/pkg/rbac/hierarchy.gocomponents/ambient-api-server/pkg/rbac/middleware.gocomponents/ambient-api-server/pkg/rbac/permissions.gocomponents/ambient-api-server/pkg/rbac/scope.gocomponents/ambient-api-server/plugins/applications/dao.gocomponents/ambient-api-server/plugins/applications/factory_test.gocomponents/ambient-api-server/plugins/applications/handler.gocomponents/ambient-api-server/plugins/applications/integration_test.gocomponents/ambient-api-server/plugins/applications/migration.gocomponents/ambient-api-server/plugins/applications/mock_dao.gocomponents/ambient-api-server/plugins/applications/model.gocomponents/ambient-api-server/plugins/applications/plugin.gocomponents/ambient-api-server/plugins/applications/presenter.gocomponents/ambient-api-server/plugins/applications/service.gocomponents/ambient-api-server/plugins/applications/testmain_test.gocomponents/ambient-cli/cmd/acpctl/application/cmd.gocomponents/ambient-cli/cmd/acpctl/create/cmd.gocomponents/ambient-cli/cmd/acpctl/delete/cmd.gocomponents/ambient-cli/cmd/acpctl/describe/cmd.gocomponents/ambient-cli/cmd/acpctl/get/cmd.gocomponents/ambient-cli/cmd/acpctl/main.gocomponents/ambient-cli/cmd/acpctl/session/messages.gocomponents/ambient-sdk/go-sdk/client/agent_api.gocomponents/ambient-sdk/go-sdk/client/application_api.gocomponents/ambient-sdk/go-sdk/client/client.gocomponents/ambient-sdk/go-sdk/client/credential_api.gocomponents/ambient-sdk/go-sdk/client/inbox_message_api.gocomponents/ambient-sdk/go-sdk/client/iterator.gocomponents/ambient-sdk/go-sdk/client/project_api.gocomponents/ambient-sdk/go-sdk/client/project_settings_api.gocomponents/ambient-sdk/go-sdk/client/role_api.gocomponents/ambient-sdk/go-sdk/client/role_binding_api.gocomponents/ambient-sdk/go-sdk/client/scheduled_session_api.gocomponents/ambient-sdk/go-sdk/client/session_api.gocomponents/ambient-sdk/go-sdk/client/session_message_api.gocomponents/ambient-sdk/go-sdk/client/user_api.gocomponents/ambient-sdk/go-sdk/types/agent.gocomponents/ambient-sdk/go-sdk/types/application.gocomponents/ambient-sdk/go-sdk/types/base.gocomponents/ambient-sdk/go-sdk/types/credential.gocomponents/ambient-sdk/go-sdk/types/inbox_message.gocomponents/ambient-sdk/go-sdk/types/list_options.gocomponents/ambient-sdk/go-sdk/types/project.gocomponents/ambient-sdk/go-sdk/types/project_settings.gocomponents/ambient-sdk/go-sdk/types/role.gocomponents/ambient-sdk/go-sdk/types/role_binding.gocomponents/ambient-sdk/go-sdk/types/scheduled_session.gocomponents/ambient-sdk/go-sdk/types/session.gocomponents/ambient-sdk/go-sdk/types/session_message.gocomponents/ambient-sdk/go-sdk/types/user.gocomponents/ambient-sdk/python-sdk/ambient_platform/__init__.pycomponents/ambient-sdk/python-sdk/ambient_platform/_agent_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_application_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_base.pycomponents/ambient-sdk/python-sdk/ambient_platform/_credential_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_inbox_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_iterator.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_project_settings_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_role_binding_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_scheduled_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_session_message_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/_user_api.pycomponents/ambient-sdk/python-sdk/ambient_platform/agent.pycomponents/ambient-sdk/python-sdk/ambient_platform/application.pycomponents/ambient-sdk/python-sdk/ambient_platform/client.pycomponents/ambient-sdk/python-sdk/ambient_platform/credential.pycomponents/ambient-sdk/python-sdk/ambient_platform/inbox_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/project.pycomponents/ambient-sdk/python-sdk/ambient_platform/project_settings.pycomponents/ambient-sdk/python-sdk/ambient_platform/role.pycomponents/ambient-sdk/python-sdk/ambient_platform/role_binding.pycomponents/ambient-sdk/python-sdk/ambient_platform/scheduled_session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session.pycomponents/ambient-sdk/python-sdk/ambient_platform/session_message.pycomponents/ambient-sdk/python-sdk/ambient_platform/user.pycomponents/ambient-sdk/ts-sdk/src/agent.tscomponents/ambient-sdk/ts-sdk/src/agent_api.tscomponents/ambient-sdk/ts-sdk/src/application.tscomponents/ambient-sdk/ts-sdk/src/application_api.tscomponents/ambient-sdk/ts-sdk/src/base.tscomponents/ambient-sdk/ts-sdk/src/client.tscomponents/ambient-sdk/ts-sdk/src/credential.tscomponents/ambient-sdk/ts-sdk/src/credential_api.tscomponents/ambient-sdk/ts-sdk/src/inbox_message.tscomponents/ambient-sdk/ts-sdk/src/inbox_message_api.tscomponents/ambient-sdk/ts-sdk/src/index.tscomponents/ambient-sdk/ts-sdk/src/project.tscomponents/ambient-sdk/ts-sdk/src/project_api.tscomponents/ambient-sdk/ts-sdk/src/project_settings.tscomponents/ambient-sdk/ts-sdk/src/project_settings_api.tscomponents/ambient-sdk/ts-sdk/src/role.tscomponents/ambient-sdk/ts-sdk/src/role_api.tscomponents/ambient-sdk/ts-sdk/src/role_binding.tscomponents/ambient-sdk/ts-sdk/src/role_binding_api.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session.tscomponents/ambient-sdk/ts-sdk/src/scheduled_session_api.tscomponents/ambient-sdk/ts-sdk/src/session.tscomponents/ambient-sdk/ts-sdk/src/session_api.tscomponents/ambient-sdk/ts-sdk/src/session_message.tscomponents/ambient-sdk/ts-sdk/src/session_message_api.tscomponents/ambient-sdk/ts-sdk/src/user.tscomponents/ambient-sdk/ts-sdk/src/user_api.tsworkflows/sessions/ambient-model.workflow.md
✅ Files skipped from review due to trivial changes (71)
- components/ambient-sdk/ts-sdk/src/project_settings.ts
- components/ambient-sdk/go-sdk/types/list_options.go
- components/ambient-sdk/python-sdk/ambient_platform/role_binding.py
- components/ambient-sdk/go-sdk/types/credential.go
- components/ambient-sdk/ts-sdk/src/user_api.ts
- components/ambient-sdk/go-sdk/client/session_api.go
- components/ambient-sdk/python-sdk/ambient_platform/agent.py
- components/ambient-sdk/ts-sdk/src/inbox_message.ts
- components/ambient-sdk/python-sdk/ambient_platform/project_settings.py
- components/ambient-sdk/ts-sdk/src/project.ts
- components/ambient-sdk/ts-sdk/src/base.ts
- components/ambient-sdk/python-sdk/ambient_platform/_session_api.py
- components/ambient-sdk/go-sdk/types/agent.go
- components/ambient-sdk/go-sdk/client/agent_api.go
- components/ambient-sdk/python-sdk/ambient_platform/_project_api.py
- components/ambient-sdk/go-sdk/client/user_api.go
- components/ambient-sdk/go-sdk/client/credential_api.go
- components/ambient-sdk/ts-sdk/src/agent_api.ts
- components/ambient-sdk/ts-sdk/src/project_settings_api.ts
- components/ambient-sdk/ts-sdk/src/scheduled_session.ts
- components/ambient-sdk/ts-sdk/src/credential_api.ts
- components/ambient-sdk/ts-sdk/src/role.ts
- components/ambient-sdk/ts-sdk/src/role_api.ts
- components/ambient-sdk/go-sdk/types/user.go
- components/ambient-sdk/ts-sdk/src/project_api.ts
- components/ambient-sdk/go-sdk/types/project.go
- components/ambient-sdk/go-sdk/types/project_settings.go
- components/ambient-sdk/go-sdk/client/inbox_message_api.go
- components/ambient-sdk/ts-sdk/src/role_binding.ts
- components/ambient-sdk/go-sdk/types/inbox_message.go
- components/ambient-sdk/go-sdk/client/project_api.go
- components/ambient-sdk/python-sdk/ambient_platform/_inbox_message_api.py
- components/ambient-sdk/go-sdk/client/scheduled_session_api.go
- components/ambient-sdk/go-sdk/client/session_message_api.go
- components/ambient-sdk/ts-sdk/src/session_api.ts
- components/ambient-sdk/ts-sdk/src/session_message.ts
- components/ambient-sdk/python-sdk/ambient_platform/_agent_api.py
- components/ambient-sdk/python-sdk/ambient_platform/credential.py
- components/ambient-sdk/ts-sdk/src/inbox_message_api.ts
- components/ambient-sdk/ts-sdk/src/role_binding_api.ts
- components/ambient-sdk/python-sdk/ambient_platform/_scheduled_session_api.py
- components/ambient-sdk/python-sdk/ambient_platform/project.py
- components/ambient-sdk/ts-sdk/src/session_message_api.ts
- components/ambient-sdk/ts-sdk/src/scheduled_session_api.ts
- components/ambient-sdk/python-sdk/ambient_platform/_base.py
- components/ambient-sdk/ts-sdk/src/credential.ts
- components/ambient-sdk/python-sdk/ambient_platform/_role_api.py
- components/ambient-sdk/python-sdk/ambient_platform/_session_message_api.py
- components/ambient-sdk/ts-sdk/src/agent.ts
- components/ambient-sdk/python-sdk/ambient_platform/session_message.py
- components/ambient-sdk/ts-sdk/src/session.ts
- components/ambient-sdk/python-sdk/ambient_platform/role.py
- components/ambient-sdk/python-sdk/ambient_platform/_project_settings_api.py
- components/ambient-sdk/python-sdk/ambient_platform/_iterator.py
- components/ambient-sdk/go-sdk/client/project_settings_api.go
- components/ambient-sdk/go-sdk/client/iterator.go
- components/ambient-sdk/python-sdk/ambient_platform/_credential_api.py
- components/ambient-sdk/python-sdk/ambient_platform/session.py
- components/ambient-sdk/go-sdk/client/role_api.go
- components/ambient-sdk/python-sdk/ambient_platform/_application_api.py
- components/ambient-sdk/python-sdk/ambient_platform/scheduled_session.py
- components/ambient-sdk/ts-sdk/src/application_api.ts
- components/ambient-sdk/go-sdk/types/base.go
- components/ambient-sdk/python-sdk/ambient_platform/_role_binding_api.py
- components/ambient-sdk/go-sdk/types/role_binding.go
- components/ambient-sdk/go-sdk/client/application_api.go
- components/ambient-sdk/go-sdk/types/application.go
- components/ambient-sdk/go-sdk/types/session.go
- components/ambient-sdk/python-sdk/ambient_platform/inbox_message.py
- workflows/sessions/ambient-model.workflow.md
- components/ambient-sdk/python-sdk/ambient_platform/application.py
🚧 Files skipped from review as they are similar to previous changes (30)
- components/ambient-sdk/go-sdk/types/session_message.go
- components/ambient-sdk/go-sdk/client/role_binding_api.go
- components/ambient-sdk/ts-sdk/src/user.ts
- components/ambient-sdk/go-sdk/client/client.go
- components/ambient-sdk/go-sdk/types/scheduled_session.go
- components/ambient-sdk/ts-sdk/src/client.ts
- components/ambient-api-server/pkg/rbac/hierarchy.go
- components/ambient-sdk/python-sdk/ambient_platform/_user_api.py
- components/ambient-api-server/plugins/applications/migration.go
- components/ambient-sdk/python-sdk/ambient_platform/user.py
- components/ambient-api-server/cmd/ambient-api-server/main.go
- components/ambient-api-server/pkg/rbac/permissions.go
- components/ambient-api-server/pkg/rbac/middleware.go
- components/ambient-sdk/go-sdk/types/role.go
- components/ambient-cli/cmd/acpctl/describe/cmd.go
- components/ambient-sdk/ts-sdk/src/index.ts
- components/ambient-api-server/plugins/applications/testmain_test.go
- components/ambient-api-server/plugins/applications/plugin.go
- components/ambient-sdk/ts-sdk/src/application.ts
- components/ambient-api-server/pkg/rbac/scope.go
- components/ambient-api-server/openapi/openapi.yaml
- components/ambient-sdk/python-sdk/ambient_platform/client.py
- components/ambient-api-server/openapi/openapi.applications.yaml
- components/ambient-sdk/python-sdk/ambient_platform/init.py
- components/ambient-api-server/plugins/applications/presenter.go
- components/ambient-api-server/plugins/applications/integration_test.go
- components/ambient-api-server/plugins/applications/service.go
- components/ambient-cli/cmd/acpctl/create/cmd.go
- components/ambient-api-server/plugins/applications/handler.go
- components/ambient-api-server/plugins/applications/dao.go
| func (d *applicationDaoMock) Replace(ctx context.Context, application *Application) (*Application, error) { | ||
| return nil, errors.NotImplemented("Application").AsError() | ||
| } | ||
|
|
||
| func (d *applicationDaoMock) Delete(ctx context.Context, id string) error { | ||
| return errors.NotImplemented("Application").AsError() | ||
| } | ||
|
|
||
| func (d *applicationDaoMock) FindByIDs(ctx context.Context, ids []string) (ApplicationList, error) { | ||
| return nil, errors.NotImplemented("Application").AsError() | ||
| } |
There was a problem hiding this comment.
Implement stateful Replace/Delete/FindByIDs in the mock DAO (Line 35 onward).
NotImplemented here makes key application flows untestable with the mock: Patch requires Replace (handler), and delete paths require Delete (service). This violates the mock DAO contract for unit tests.
Suggested minimal in-memory implementation
func (d *applicationDaoMock) Replace(ctx context.Context, application *Application) (*Application, error) {
- return nil, errors.NotImplemented("Application").AsError()
+ for i, existing := range d.applications {
+ if existing.ID == application.ID {
+ d.applications[i] = application
+ return application, nil
+ }
+ }
+ return nil, gorm.ErrRecordNotFound
}
func (d *applicationDaoMock) Delete(ctx context.Context, id string) error {
- return errors.NotImplemented("Application").AsError()
+ for i, existing := range d.applications {
+ if existing.ID == id {
+ d.applications = append(d.applications[:i], d.applications[i+1:]...)
+ return nil
+ }
+ }
+ return gorm.ErrRecordNotFound
}
func (d *applicationDaoMock) FindByIDs(ctx context.Context, ids []string) (ApplicationList, error) {
- return nil, errors.NotImplemented("Application").AsError()
+ index := map[string]struct{}{}
+ for _, id := range ids {
+ index[id] = struct{}{}
+ }
+ var out ApplicationList
+ for _, application := range d.applications {
+ if _, ok := index[application.ID]; ok {
+ out = append(out, application)
+ }
+ }
+ return out, nil
}As per coding guidelines, components/ambient-api-server/plugins/*/mock_dao.go: “Mock DAO files must provide mock implementations for unit tests.”
📝 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.
| func (d *applicationDaoMock) Replace(ctx context.Context, application *Application) (*Application, error) { | |
| return nil, errors.NotImplemented("Application").AsError() | |
| } | |
| func (d *applicationDaoMock) Delete(ctx context.Context, id string) error { | |
| return errors.NotImplemented("Application").AsError() | |
| } | |
| func (d *applicationDaoMock) FindByIDs(ctx context.Context, ids []string) (ApplicationList, error) { | |
| return nil, errors.NotImplemented("Application").AsError() | |
| } | |
| func (d *applicationDaoMock) Replace(ctx context.Context, application *Application) (*Application, error) { | |
| for i, existing := range d.applications { | |
| if existing.ID == application.ID { | |
| d.applications[i] = application | |
| return application, nil | |
| } | |
| } | |
| return nil, gorm.ErrRecordNotFound | |
| } | |
| func (d *applicationDaoMock) Delete(ctx context.Context, id string) error { | |
| for i, existing := range d.applications { | |
| if existing.ID == id { | |
| d.applications = append(d.applications[:i], d.applications[i+1:]...) | |
| return nil | |
| } | |
| } | |
| return gorm.ErrRecordNotFound | |
| } | |
| func (d *applicationDaoMock) FindByIDs(ctx context.Context, ids []string) (ApplicationList, error) { | |
| index := map[string]struct{}{} | |
| for _, id := range ids { | |
| index[id] = struct{}{} | |
| } | |
| var out ApplicationList | |
| for _, application := range d.applications { | |
| if _, ok := index[application.ID]; ok { | |
| out = append(out, application) | |
| } | |
| } | |
| return out, nil | |
| } |
🤖 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 `@components/ambient-api-server/plugins/applications/mock_dao.go` around lines
35 - 45, The mock DAO currently returns NotImplemented for Replace, Delete, and
FindByIDs; implement stateful in-memory behavior on applicationDaoMock by
adding/using an internal map[string]*Application (or map[string]Application) and
appropriate locking if concurrency is possible; implement Replace(ctx,
application *Application) to upsert the application into the map and return the
stored copy, Delete(ctx, id string) to remove the entry and return nil or a
not-found error consistent with other mocks, and FindByIDs(ctx, ids []string) to
gather and return an ApplicationList in the same order as ids (omitting or
returning placeholders for missing entries per mock convention); update
applicationDaoMock methods Replace, Delete, and FindByIDs to use that map and
ensure results match expected types.
Source: Coding guidelines
Summary
gitops:admin,gitops:viewer)ApplicationBuilderandApplicationPatchBuilderfor type-safe resource constructionacpctl applicationcommand with 7 subcommands (list/get/create/update/delete/sync/refresh) plus registration in all generic commands (get,create,delete,describe)ResourceApplicationpermission constants, role hierarchy entries, scope/middleware updates forsync/refresh→updateaction mappingImplements the Application kind (GitOps continuous sync) from the data model spec in #1648.
Components Changed
< /dev/null | Component | Changes |
|---|---|
|
ambient-api-server| OpenAPI spec, plugin (model/dao/service/handler/presenter/migration), RBAC extensions, integration tests ||
ambient-sdk| Generated Go/Python/TS clients and types for Application CRUD ||
ambient-cli| Standaloneapplicationcommand + genericget/create/delete/describesupport ||
workflows| 11 lessons learned added toambient-model.workflow.md|Test plan
go build ./...passes for api-server, cli, and go-sdkgo vet ./...passes for api-server and clipanic()in production codeanytypes in frontend/TS codeacpctl application --helpverified with all subcommandsget applications,delete application,describe application,create application) all verified🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
SDKs
Security / Roles
Database
Tests
Documentation