NotificationsYou must be signed in to change notification settings
Fork905
Star10k

feat(coderd/agentapi): allow inserting apps for sub agents#18129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

DanielleMaywood merged 19 commits intomainfromdm-sub-agent-workspace-apps

Jun 5, 2025

Merged

feat(coderd/agentapi): allow inserting apps for sub agents#18129

DanielleMaywood merged 19 commits intomainfromdm-sub-agent-workspace-apps

Jun 5, 2025

Conversation

Copy link

Contributor

DanielleMaywood commentedMay 30, 2025•
edited
Loading

Relates tocoder/internal#614

Introduces a new field to theCreateSubAgent request. This new field allows adding workspace apps to a sub agent at the point of creation.

DanielleMaywood added2 commits

May 30, 2025 10:48

feat(coderd/agentapi): allow inserting workspace apps for subagents

6acef6a

Allow the creation of workspace apps when creating a sub agent.

chore: revert dbauthz change

07d86b5

I think the dbauthz change is out-of-scope for this change

github-actionsbot assignedDanielleMaywood

May 30, 2025

DanielleMaywood commented

May 30, 2025

View reviewed changes

agent/proto/agent.proto

Comment on lines +392 to +423

		message App {
		message Healthcheck {
		int32 interval = 1;
		int32 threshold = 2;
		string url = 3;
		}

		enum OpenIn {
		SLIM_WINDOW = 0;
		TAB = 1;
		}

		enum Share {
		OWNER = 0;
		AUTHENTICATED = 1;
		PUBLIC = 2;
		}

		string slug = 1;
		optional string command = 2;
		optional string display_name = 3;
		optional bool external = 4;
		optional string group = 5;
		optional Healthcheck healthcheck = 6;
		optional bool hidden = 7;
		optional string icon = 8;
		optional OpenIn open_in = 9;
		optional int32 order = 10;
		optional Share share = 11;
		optional bool subdomain = 12;
		optional string url = 13;
		}

Copy link

ContributorAuthor

DanielleMaywoodMay 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm aware we already have aWorkspaceApp structure defined inagent.proto, but that is used as part of theManifest response structure.

The reasoning for creating a new structure is that the existingWorkspaceApp is for an existing workspace app, whereas this new version is for requesting to make anew workspace app.

The design of this structure is based on the terraform providercoder_app resource.

coderd/agentapi/subagent.go OutdatedShow resolvedHide resolved

chore: appease linter

a144b8d

DanielleMaywood requested a review fromCopilot

May 30, 2025 15:27

CopilotAI reviewed

May 30, 2025

View reviewed changes

Copy link

Contributor

CopilotAI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Pull Request Overview

This PR adds the ability to insert apps when creating sub agents by extending the API, database logic, and corresponding proto definitions.

Added a new test case ("CreateSubAgentWithApps") validating sub-agent creation with associated apps.
Updated the CreateSubAgent function to iterate over and insert apps with validations.
Extended the proto definition to include a new App message with nested Healthcheck and enums for OpenIn and Share.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File	Description
coderd/agentapi/subagent_test.go	Added tests for creating sub agents including the apps parameter with various scenarios.
coderd/agentapi/subagent.go	Added business logic to process and insert the apps passed via the CreateSubAgent request.
agent/proto/agent.proto	Extended the proto message for CreateSubAgentRequest with a nested App message and related fields.

DanielleMaywood added5 commits

May 30, 2025 15:35

chore: add some more tests

493a415

Merge branch 'main' into dm-sub-agent-workspace-apps

26fa15f

Merge branch 'main' into dm-sub-agent-workspace-apps

62c8a4c

test: workspace apps are deleted

4592191

Merge branch 'main' into dm-sub-agent-workspace-apps

a04bf06

DanielleMaywood marked this pull request as ready for review

June 3, 2025 10:52

DanielleMaywood requested review fromspikecurtis andjohnstcn ascode owners

June 3, 2025 10:52

DanielleMaywood requested a review frommafredri

June 3, 2025 10:52

johnstcn reviewed

Jun 3, 2025

View reviewed changes

Copy link

Member

johnstcn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Because you add new types here, we should also update the comments intailnet/proto/version.go to reflect this. It hasn't been shipped yet AFAIK so it should be OK to just keep the same version.

The other potential issue I see here is correctly surfacing validation errors from theCreateSubAgent() calls to the user. I don't see any calls to this yet, but we'll want to make sure that users are able to correctly address issues that cause validation to fail.

coderd/agentapi/subagent.go OutdatedShow resolvedHide resolved

coderd/agentapi/subagent_test.go OutdatedShow resolvedHide resolved

coderd/agentapi/subagent.go OutdatedShow resolvedHide resolved

DanielleMaywood added3 commits

June 3, 2025 16:21

refactor: return codersdk.ValidationError

5512cba

Instead of returning an untyped error, we instead return a typed error.This will allow matching on the error at the call site, allowing it tobe dispayed nicely to the user.

chore: appease linter, add todo

7354e2a

fix: add validation error for duplicate slug names

5250e1c

Copy link

ContributorAuthor

DanielleMaywood commentedJun 3, 2025•
edited
Loading

Because you add new types here, we should also update the comments intailnet/proto/version.go to reflect this. It hasn't been shipped yet AFAIK so it should be OK to just keep the same version.

@johnstcn As the new types added are all under theCreateSubAgentRequest message, and that message was added in the unreleased version2.6, I had assumed it didn't need to be added. I'm happy to make the change though if needed?

Copy link

Member

johnstcn commentedJun 4, 2025

As the new types added are all under theCreateSubAgentRequest message, and that message was added in the unreleased version2.6, I had assumed it didn't need to be added. I'm happy to make the change though if needed?

@DanielleMaywood fine with me 👍

johnstcn approved these changes

Jun 4, 2025

View reviewed changes

DanielleMaywood added3 commits

June 4, 2025 11:23

chore: remove TODO and AsSystemRestricted

360e667

Merge branch 'main' into dm-sub-agent-workspace-apps

fa89ddb

chore: oops, forgot to add ActionUpdate to permissions

65495fe

mafredri reviewed

Jun 4, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I think we should utilize transactions here, and I'm a bit torn whether or not to error for app creation. And if we don't error, how do we surface the problem? (Perhaps via some field on the response.)

agent/proto/agent.protoShow resolvedHide resolved

coderd/agentapi/subagent.go OutdatedShow resolvedHide resolved

coderd/agentapi/subagent.go

		})
		if err != nil {
		return nil, xerrors.Errorf("insert workspace app: %w", err)
		}

Copy link

Member

mafredriJun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

There are a lot of paths where we may error out here, do we want to prevent sub agent creation even if an app is wrongly configured? This makes backwards compatibility also harder where if we ever have to change the customize structure, sub agents may start failing rather than individual apps.

Copy link

ContributorAuthor

DanielleMaywoodJun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I can definitely see where you're coming from though I'm not sure which approach is the best.@johnstcn what are your thoughts here?

Copy link

ContributorAuthor

DanielleMaywoodJun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Have made it so the creation of an agent will not fail if an app fails to be created 👍

DanielleMaywood added3 commits

June 4, 2025 11:56

chore: wrap in a transaction

c281c77

Wrap the calls to InsertWorkspaceAgent and InsertWorkspaceApp in atransaction so that they either all succeed or none succeed.

refactor: allow agent to be created when apps fail to be

8137a65

This change allows an agent to still be created and returned, even whenan app fails to be created. This is made possible by returning eachindividual app creation error.

chore: appease linter and formatter

fdedcd1

DanielleMaywood requested a review frommafredri

June 5, 2025 09:52

mafredri approved these changes

Jun 5, 2025

View reviewed changes

Copy link

Member

mafredri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Only had one suggestion for a bit of defensive programming, but otherwise this LGTM, nice work!

coderd/agentapi/subagent.go Outdated

Comment on lines 110 to 129

		health := database.WorkspaceAppHealthDisabled
		if app.Healthcheck == nil {
		app.Healthcheck = &agentproto.CreateSubAgentRequest_App_Healthcheck{}
		}
		if app.Healthcheck.Url != "" {
		health = database.WorkspaceAppHealthInitializing
		}

		sharingLevel := database.AppSharingLevelOwner
		switch app.GetShare() {
		case agentproto.CreateSubAgentRequest_App_AUTHENTICATED:
		sharingLevel = database.AppSharingLevelAuthenticated
		case agentproto.CreateSubAgentRequest_App_PUBLIC:
		sharingLevel = database.AppSharingLevelPublic
		}

		openIn := database.WorkspaceAppOpenInSlimWindow
		if app.GetOpenIn() == agentproto.CreateSubAgentRequest_App_TAB {
		openIn = database.WorkspaceAppOpenInTab
		}

Copy link

Member

mafredriJun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Perhaps we should skip the default and haveswitch for each type with a default that returns an error? Thinking about future changes to the API and potential silent breakages/incorrect behavior that will go unnoticed otherwise.

coderd/agentapi/subagent.go

		}()
		if err != nil {
		appErr := &agentproto.CreateSubAgentResponse_AppCreationError{
		Index: int32(i), //nolint:gosec // This would only overflow if we created 2 billion apps.

Copy link

Member

mafredriJun 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

😄

DanielleMaywood added2 commits

June 5, 2025 10:30

chore: do some more defensive programming

c8916e1

fix: OOPS

b4cf1f3

DanielleMaywood merged commitb5fd3dd intomain

Jun 5, 2025

34 checks passed

DanielleMaywood deleted the dm-sub-agent-workspace-apps branch

June 5, 2025 10:57

github-actionsbot locked and limited conversation to collaborators

Jun 5, 2025

Labels

None yet

4 participants

Movatterモバイル変換

feat(coderd/agentapi): allow inserting apps for sub agents#18129

feat(coderd/agentapi): allow inserting apps for sub agents#18129

Uh oh!

Conversation

DanielleMaywood commentedMay 30, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

DanielleMaywoodMay 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

johnstcn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielleMaywood commentedJun 3, 2025• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

johnstcn commentedJun 4, 2025

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mafredriJun 4, 2025

Choose a reason for hiding this comment

Uh oh!

DanielleMaywoodJun 4, 2025

Choose a reason for hiding this comment

Uh oh!

DanielleMaywoodJun 5, 2025

Choose a reason for hiding this comment

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

mafredriJun 5, 2025

Choose a reason for hiding this comment

Uh oh!

mafredriJun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

DanielleMaywood commentedMay 30, 2025•
edited
Loading

DanielleMaywood commentedJun 3, 2025•
edited
Loading