This definitely isn't an ideal test. It feels quite large. Maybe I'lltry to extract the different things being tested and move them intotheir own tests.

refactor: tests

7fd59db

Refactor the One Big Test into multiple separate unit tests

chore: appease linter

b1f8e42

chore: begin validating agent name

eb881ea

Begin validation of the agent name. This uses the same logic (with theexception of duplicate agent name detection) as the provisioner.A future commit will aim to rectify the duplicate agent name issue.

DanielleMaywood force-pushed thedm-devcontainer-agents-coderd branch fromc18f28a toeb881eaCompare

May 21, 2025 15:19

DanielleMaywood requested a review fromCopilot

May 21, 2025 15:19

CopilotAI reviewed

May 21, 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 implements a new Dev Container Agent API for managing workspace agents used by development containers in the system. Key changes include:

Adding a new subject type and authorization support for dev container agents.
Introducing SQL queries, querier methods, in-memory and mock DB implementations for workspace agent deletion and retrieval by parent ID.
Creating API endpoints and corresponding proto definitions/methods for creating, deleting, and listing dev container agents.

Reviewed Changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
coderd/rbac/authz.go	Introduces a new subject type for dev container agent API authorization.
coderd/database/queries/workspaceagents.sql	Adds SQL queries for workspace agent deletion and retrieval by parent ID.
coderd/database/queries.sql.go	Adds new SQL constants and functions for workspace agent operations.
coderd/database/querier.go	Updates the querier interface with new workspace agent methods.
coderd/database/dbmock/dbmock.go	Updates mock implementations to support new workspace agent methods.
coderd/database/dbmetrics/querymetrics.go	Adds metrics wrappers for the new workspace agent queries.
coderd/database/dbmem/dbmem.go	Implements fake methods for deleting and retrieving workspace agents.
coderd/database/dbauthz/dbauthz_test.go	Adds tests for workspace agent authorization actions.
coderd/database/dbauthz/dbauthz.go	Implements authorization logic and API methods for workspace agent deletion/retrieval.
coderd/agentapi/devcontainer_agent_test.go	Provides tests for creating, deleting, and listing dev container agents.
coderd/agentapi/devcontainer_agent.go	Implements the Dev Container Agent API endpoints.
coderd/agentapi/api.go	Integrates the Dev Container Agent API into the main API composition.
agent/proto/agent_drpc.pb.go	Updates generated proto code with new DRPC client/server methods.
agent/proto/agent.proto	Defines new RPC messages and endpoints for dev container agent operations.
agent/agenttest/client.go	Adds stub implementations for new dev container agent methods in the fake client.

coderd/database/dbmem/dbmem.go OutdatedShow resolvedHide resolved

chore: listen to feedback

924699c

DanielleMaywood requested a review frommafredri

May 22, 2025 12:13

mafredri reviewed

May 22, 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.

This largely looks good to me, nice work!

Just so I'm understanding this correctly. This permission model works because theagentapi is unique for each agent talking to it and you can't fake being another agent. Hence the simple queries which only filter onid/parent_id, etc. are sufficient. And on top of this, dbauthz verifies that agent is allowed to modify the workspace.

agent/proto/agent.proto OutdatedShow resolvedHide resolved

coderd/agentapi/devcontainer_agent.go OutdatedShow resolvedHide resolved

coderd/agentapi/devcontainer_agent.go Outdated

		AuthToken: uuid.New(),
		AuthInstanceID: parentAgent.AuthInstanceID,
		Architecture: req.Architecture,
		EnvironmentVariables: pqtype.NullRawMessage{},

Copy link

Member

mafredriMay 22, 2025

Choose a reason for hiding this comment

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

Note: I think this is correct, at least for now. If customers request inheritance we can look at it then, but ideally any envs needed are defined indevcontainer.json

Copy link

ContributorAuthor

DanielleMaywoodMay 22, 2025

Choose a reason for hiding this comment

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

Yeah we should definitely keep this in mind, or at least make sure to document that these variables aren't inherited.

coderd/agentapi/devcontainer_agent.go OutdatedShow resolvedHide resolved

coderd/agentapi/devcontainer_agent_test.go OutdatedShow resolvedHide resolved

coderd/database/queries/workspaceagents.sql OutdatedShow resolvedHide resolved

DanielleMaywood added8 commits

May 22, 2025 12:56

chore: rename GetWorkspaceAgentsWithParentID

b60d42d

chore: ensure createdAt and updatedAt match

505c0cc

chore: rbac

1af33bc

chore: appease formatter

1731e5f

tests: refactor and add some adversarial tests

7b06e6f

test: behavior is correct with no child agents

0c37a92

chore: appease linter

ef5772d

Merge branch 'main' into dm-devcontainer-agents-coderd

cb89e03

chore: update DRPCAgentClient25 interface

94a2d59

DanielleMaywood requested a review fromCopilot

May 22, 2025 17:22

CopilotAI reviewed

May 22, 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 implements the devcontainer agents API to support creating, deleting, and listing dev container agents while updating the corresponding database queries, authorization logic, and RPC interfaces.

Introduces new RPC endpoints and protobuf definitions for managing dev container agents.
Adds SQL queries, database functions, and mocks, as well as updates to RBAC, logging middleware, and tests to support the new API.

Reviewed Changes

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

Show a summary per file

File	Description
coderd/workspaceagentsrpc.go	Adds OrganizationID to the RPC payload for workspace agents.
coderd/rbac/authz.go	Introduces a new subject type for the dev container agent API.
coderd/httpmw/loggermw/logger.go	Updates logger middleware to accommodate the new subject type.
coderd/database/queries/*.sql	Adds new SQL queries for fetching agents by parent and deleting agents.
coderd/database/queries.sql.go	Implements new constants and methods for the workspace agent queries.
coderd/database/querier.go	Adds new querier interface methods for dev container agent operations.
coderd/database/dbmock/dbmock.go	Adds mocks for the new database methods.
coderd/database/dbmetrics/querymetrics.go	Includes metrics wrappers for the new database operations.
coderd/database/dbmem/dbmem.go	Implements in-memory versions of the new database methods.
coderd/database/dbauthz/dbauthz_test.go	Introduces tests to verify authorization for the new API methods.
coderd/database/dbauthz/dbauthz.go	Adds authorization logic and a helper for dev container agent API.
coderd/agentapi/devcontainer_agent.go	Implements the new dev container agent API endpoints.
coderd/agentapi/api.go	Integrates DevContainerAgentAPI into the main API.
agent/proto/agent.proto, agent_drpc*.go	Updates prototype and DRPC definitions to add the new RPC endpoints.
agent/agenttest/client.go	Adds stub (unimplemented) methods for the new dev container API.

Comments suppressed due to low confidence (2)

coderd/database/queries.sql.go:14583

[nitpick] Consider renaming the parameter 'dollar_1' to a more descriptive name (e.g., 'parentID') to improve clarity.

func (q *sqlQuerier) GetWorkspaceAgentsByParentID(ctx context.Context, dollar_1 uuid.UUID) ([]WorkspaceAgent, error) {

coderd/database/querier.go:416

[nitpick] Consider renaming the parameter 'dollar_1' to a more meaningful name (e.g., 'parentID') for better readability and consistency.

GetWorkspaceAgentsByParentID(ctx context.Context, dollar_1 uuid.UUID) ([]WorkspaceAgent, error)

chore: rename sql parameter

ec4dda9

DanielleMaywood force-pushed thedm-devcontainer-agents-coderd branch fromb8675f2 toec4dda9Compare

May 22, 2025 17:25

DanielleMaywood added3 commits

May 27, 2025 09:10

Merge branch 'main' into dm-devcontainer-agents-coderd

0c8be91

Merge branch 'main' into dm-devcontainer-agents-coderd

34d870a

chore: remove comment as recent PR resolves issue

cb965ea

DanielleMaywood commented

May 28, 2025

View reviewed changes

agent/proto/agent_drpc_old.go OutdatedShow resolvedHide resolved

DanielleMaywood marked this pull request as ready for review

May 28, 2025 13:40

DanielleMaywood requested review fromspikecurtis andjohnstcn ascode owners

May 28, 2025 13:40

DanielleMaywood requested a review frommafredri

May 28, 2025 13:40

mafredri approved these changes

May 28, 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.

This looks largely good IMO. I suggest adding a bit more paranoia to the agent deletion query, but otherwise only minor suggestions and discussion points.

Pre-approving but deferring final approval to@spikecurtis and@johnstcn.

agent/proto/agent_drpc_old.go OutdatedShow resolvedHide resolved

coderd/agentapi/api.go OutdatedShow resolvedHide resolved

coderd/agentapi/devcontainer_agent.go Outdated

		for i, agent := range workspaceAgents {
		agents[i] = &agentproto.ListDevContainerAgentsResponse_DevContainerAgent{
		Name: agent.Name,
		Id: agent.ID[:],

Copy link

Member

mafredriMay 28, 2025

Choose a reason for hiding this comment

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

Discussion: Since we don't output a token here, if the agent ever crashes and is restarted, I suppose it will have to start by querying this list, deleting each agent, and then create new ones. I think that's probably best, but thought I'd raise this anyway.

coderd/database/dbauthz/dbauthz.go Outdated

		@@ -319,6 +319,28 @@ var (
		Scope: rbac.ScopeAll,
		}.WithCachedASTValue()

		subjectDevContainerAgentAPI = func(userID uuid.UUID, orgID uuid.UUID) rbac.Subject {

Copy link

Member

mafredriMay 28, 2025

Choose a reason for hiding this comment

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

Suggested change

	subjectDevContainerAgentAPI=func(userID uuid.UUID,orgID uuid.UUID) rbac.Subject {
	subjectDevContainerAgentAPI=func(orgID uuid.UUID,userID uuid.UUID) rbac.Subject {

I think re-ordering here would make sense (argument of larger scope, followed by smaller scope).

Copy link

Member

johnstcnMay 28, 2025

Choose a reason for hiding this comment

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

Out of scope of this PR, but there's an argument to be made for something like this:

type OrganizationID uuid.UUIDtype UserID uuid.UUID

You could also have something like:

type UserOrgID struct {  OrgID uuid.UUID  UserID uuid.UUID}subjectDevContainerAgentAPI = func(UserOrgID) rbac.Subject { ... }

For the purposes ofAsDevcontainerAgentAPI, you could maybe do something like the below, but it feels like overkill:

AsDevcontainerAgentAPI(ctx).InOrg(orgID).ForUser(userID)

We'll just need to be careful not to mix up OrgID and UserID.

Copy link

ContributorAuthor

DanielleMaywoodMay 28, 2025

Choose a reason for hiding this comment

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

I've re-ordered it. It would definitely be nice if we could have some type-safety on IDs.

coderd/database/queries/workspaceagents.sql OutdatedShow resolvedHide resolved

coderd/database/dbmem/dbmem.go OutdatedShow resolvedHide resolved

johnstcn reviewed

May 28, 2025

View reviewed changes

agent/proto/agent_drpc_old.go OutdatedShow resolvedHide resolved

coderd/agentapi/api.go OutdatedShow resolvedHide resolved

coderd/database/dbauthz/dbauthz.go Outdated

		@@ -319,6 +319,28 @@ var (
		Scope: rbac.ScopeAll,
		}.WithCachedASTValue()

		subjectDevContainerAgentAPI = func(userID uuid.UUID, orgID uuid.UUID) rbac.Subject {

Copy link

Member

johnstcnMay 28, 2025

Choose a reason for hiding this comment

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

Out of scope of this PR, but there's an argument to be made for something like this:

type OrganizationID uuid.UUIDtype UserID uuid.UUID

You could also have something like:

type UserOrgID struct {  OrgID uuid.UUID  UserID uuid.UUID}subjectDevContainerAgentAPI = func(UserOrgID) rbac.Subject { ... }

For the purposes ofAsDevcontainerAgentAPI, you could maybe do something like the below, but it feels like overkill:

AsDevcontainerAgentAPI(ctx).InOrg(orgID).ForUser(userID)

We'll just need to be careful not to mix up OrgID and UserID.

coderd/agentapi/devcontainer_agent_test.go OutdatedShow resolvedHide resolved

coderd/database/queries/workspaceagents.sql

		@@ -330,3 +330,9 @@ INNER JOIN workspace_resources ON workspace_resources.id = workspace_agents.reso
		INNER JOIN workspace_builds ON workspace_builds.job_id = workspace_resources.job_id
		WHERE workspace_builds.id = $1
		ORDER BY workspace_agent_script_timings.script_id, workspace_agent_script_timings.started_at;

		-- name: GetWorkspaceAgentsByParentID :many
		SELECT * FROM workspace_agents WHERE parent_id = @parent_id::uuid;

Copy link

Member

johnstcnMay 28, 2025

Choose a reason for hiding this comment

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

More paranoia: given thatparent_id is nullable, what happens if someone calls this withuuid.Nil? Should we do a similar check forAND parent_id IS NOT NULL?

Copy link

ContributorAuthor

DanielleMaywoodMay 28, 2025

Choose a reason for hiding this comment

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

Ifparent_id wasNULL then it should return nothing (assuming my Postgres knowledge is correct).

Copy link

ContributorAuthor

DanielleMaywoodMay 29, 2025

Choose a reason for hiding this comment

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

I've added a test to verify this behavior is what we'd want.

DanielleMaywood added5 commits

May 28, 2025 20:40

refactor: devcontainer agent -> subagent

979f4ef

Replace all naming of Dev Container Agent to Sub Agent.

refactor: deleteworkspaceagent to deleteworkspacesubagent

2ffc0e3

Refactor the DeleteWorkspaceAgentByID query to beDeleteWorkspaceSubAgentByID and add a filter that only allows it todelete an agent that has a non-NULL parent id.

fix: appease linter

f1a4da3

test: paranoia

036436b

Merge branch 'main' into dm-devcontainer-agents-coderd

c6ded1f

spikecurtis reviewed

May 29, 2025

View reviewed changes

agent/proto/agent.proto OutdatedShow resolvedHide resolved

DanielleMaywood added2 commits

May 29, 2025 09:14

chore: apply feedback from spike

0871c82

test: check err is a not authorized error

b7e64ef

johnstcn approved these changes

May 29, 2025

View reviewed changes

agent/proto/agent_drpc_old.goShow resolvedHide resolved

coderd/database/dbmem/dbmem.go OutdatedShow resolvedHide resolved

fix: remove unused code

1c28667

Accidentally left some unused code in dbmem.go

DanielleMaywood changed the title~~feat(coderd/agentapi): implement devcontainer agents api~~feat(coderd/agentapi): implement sub agent api

May 29, 2025

DanielleMaywood merged commitb712d0b intomain

May 29, 2025

36 checks passed

DanielleMaywood deleted the dm-devcontainer-agents-coderd branch

May 29, 2025 11:15

github-actionsbot locked and limited conversation to collaborators

May 29, 2025

Labels

None yet

4 participants

Movatterモバイル変換

feat(coderd/agentapi): implement sub agent api#17823

feat(coderd/agentapi): implement sub agent api#17823

Uh oh!

Conversation

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

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CopilotAI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielleMaywood commentedMay 14, 2025•
edited
Loading