fix: clamp template port sharing level in SubAgentAPI by johnstcn · Pull Request #26061 · coder/coder

johnstcn · 2026-06-04T12:53:39Z

Fixes an issue where sub-agent apps created via CreateSubAgent would bypass the check for the template's max port sharing level:

Clamps dynamically inserted workspace_apps to the template max sharing level in coderd.agentapi.SubAgentAPI.
Emits a warning when clamping occurs.
Adds unit test coverage for the max sharing level matrix.
Adds an integration-ish test through the devcontainer sub-agent client path.

Generated by Coder Agents with guidance from a human.

linear-code · 2026-06-04T12:53:43Z

CODAGT-479

johnstcn · 2026-06-04T12:56:20Z

/coder-agents-review

coder-agents-review · 2026-06-04T12:56:24Z

Chat: Review in progress | View chat
Requested: 2026-06-04 14:29 UTC by @johnstcn
Spend: $44.78 / $100.00

deep-review v0.7.1 | Round 2 | 2cbce86..4d5a974

Last posted: Round 2, 11 findings (3 P3, 5 Nit, 3 Note), APPROVE. Review

Finding inventory

Findings

#	Sev	Status	Location	Summary	Round	Reviewer	Posted
CRF-1	P3	Author fixed (`4d5a974`)	subagent.go:142	GetTemplateByID relies on template default ACL, not explicit SubAgentAPI role permission	R1	Meruem P2, Kurapika no-finding	Yes
CRF-2	P3	Author fixed (`4d5a974`)	subagent.go:225	Warn log omits sub-agent name and ID	R1	Leorio	Yes
CRF-3	P3	Author fixed (`4d5a974`)	subagent_test.go:251	require.True(HasSuffix) produces opaque failure messages	R1	Chopper P3, Bisky Nit	Yes
CRF-4	Nit	Author fixed (`4d5a974`)	subagent_test.go:43	Test name claims organization is clamped; only public is	R1	Netero	Yes
CRF-5	Nit	Author fixed (`4d5a974`)	subagent.go:225	Log says "workspace app" not "sub-agent app"	R1	Leorio	Yes
CRF-6	Note	Author accepted R2 (explicitly out of scope, aligns with reviewer's own scoping)	provisionerdserver.go:3642	Provisioner path has no MaxPortSharingLevel check	R1	Pariston	Yes
CRF-7	Note	Author fixed (`4d5a974`)	subagent.go:35	PortSharer field has no doc comment; nil fallback is permit-all	R1	Leorio	Yes
CRF-8	Note	Author fixed (`4d5a974`)	subagent.go:224	Clamping is silent; caller cannot discover the reduction	R1	Ryosuke P3, Chopper Note, Knov Note	Yes
CRF-9	Nit	Open	subagent.go:142	3-line comment could be tighter	R2	Gon P2	Yes
CRF-10	Nit	Open	subagent.go:228	Comment scopes rationale to devcontainers; handler is generic	R2	Gon P2	Yes
CRF-11	Nit	Open	subagent_test.go:340	"Coder" in integration test name adds no information	R2	Gon	Yes

Round log

Round 1

Panel. 3 P3, 2 Nit, 3 Note. Reviewed against 2cbce86..49e0119.

Round 2

Panel. CRF-1 through CRF-8 addressed. 3 Nit new. Reviewed against 2cbce86..4d5a974.

About deep-review

CRF = Coder Review Finding (P0-P4, Nit, Note)

Reviewer	Focus
Bisky	tests
Chopper	ops/errors
Churn-guard	change verification
Ging	language modernization
Gon	naming
Hisoka	edge cases
Killua	perf
Kite	change integrity
Knov	contracts
Knuckle	SQL
Kurapika	security
Law	decomposition
Leorio	docs
Luffy	product
Mafu-san	process
Mafuuu	contracts
Melody	dispatch/pairing
Meruem	structural
Nami	frontend
Netero	mechanical checks
Pariston	premise testing
Pen-botter	product gaps
Razor	verification
Robin	duplication
Ryosuke	Go arch
Takumi	concurrency
Zoro	shape

🤖 Managed by Coder Agents.

johnstcn · 2026-06-04T12:58:34Z

+	return ctx, api, &upsertedApps
+}
+
+func TestDevcontainerCoderAppShareClampedByTemplateMaxPortShareLevel(t *testing.T) {


review: chose to go with an "integration-ish" style test, avoiding the full overhead of standing up an agent with fake CLIs etc in favour of just calling the relevant DRPC methods using a stubbed client.

coder-agents-review

Clean, well-scoped fix. The clamping design (warn+restrict vs reject) is the right call for the SubAgentAPI's infrastructure callers, and the test coverage is thorough: 4 max-level boundary cases in the mock test plus a real-DB integration test through the full SubAgentClient stack. The PortSharer atomic pointer follows the established AGPL/Enterprise injection pattern. Good work.

Severity count: 3 P3, 2 Nit, 3 Note.

Pariston, after trying to build a case against this change: "I tried to build a case against this change and couldn't. The problem is correctly identified, the fix is at the right layer, the clamping behavior is the correct choice for infrastructure callers, and the tests cover the level ordering thoroughly. The premises hold."

One observation for the PR description: the current text describes the mechanism ("adds a check for and clamp") without stating the problem. Consider leading with: sub-agent apps created via CreateSubAgent bypassed the template's max port sharing level. Then the fix.

coderd/provisionerdserver/provisionerdserver.go:3642

Note [CRF-6] The provisioner maps sdkproto.AppSharingLevel to database.AppSharingLevel and stores it directly without calling AuthorizedLevel. A template with max_port_sharing_level = "authenticated" and a terraform-defined app with sharing_level = "public" stores "public" in the database. The risk is lower than the SubAgentAPI case (template admin controls both settings), but it's structurally the same gap. Not in scope for this PR, noting for visibility. (Pariston)

🤖

🤖 This review was automatically generated with Coder Agents.

johnstcn · 2026-06-04T14:12:57Z

Note: leaving CRF-6 above explicitly out of scope.

johnstcn · 2026-06-04T14:29:27Z

/coder-agents-review

coder-agents-review

All 8 R1 findings addressed in 4d5a974. Fixes verified by Netero and the full panel (9 reviewers). The comments documenting the ACL dependency, nil-PortSharer behavior, and clamp-vs-reject rationale are all accurate and well-placed. The warn log now carries sub-agent name, sub-agent ID, app slug, requested level, max level, and the underlying error, giving operators full diagnostic context.

Severity count: 3 Nit (new). All prior findings resolved.

Hisoka, after pulling every thread: "I came looking for a fight. I pulled every thread I could find. [...] No findings. The code held up."

One process note: the fix commit subject ("address automated code review feedback") is opaque, but this PR will squash-merge under the PR title, which is good.

🤖 This review was automatically generated with Coder Agents.

Copilot

Pull request overview

This PR addresses a bypass where sub-agent workspace apps created via CreateSubAgent could exceed the template’s maximum allowed port/app sharing level by adding clamping logic in coderd/agentapi.SubAgentAPI, wiring the PortSharer into the agent RPC API, and adding unit/integration-ish test coverage for sharing-level clamping.

Changes:

Add template-aware clamping for dynamically inserted workspace_apps in coderd/agentapi.SubAgentAPI, with warning logs when clamping occurs.
Thread PortSharer through agentapi.Options and the workspace agent RPC setup so SubAgentAPI can enforce enterprise sharing restrictions.
Add a new enterprise test file covering a max-sharing-level matrix and a devcontainer sub-agent client path.

Reviewed changes

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

File	Description
enterprise/coderd/subagent_test.go	Adds unit + integration-ish tests validating app share level clamping against template max.
coderd/workspaceagentsrpc.go	Passes the server `PortSharer` into the per-agent `agentapi.Options`.
coderd/agentapi/subagent.go	Fetches the workspace/template for sub-agent app creation and clamps share levels via `PortSharer`.
coderd/agentapi/api.go	Extends agent API options/sub-agent wiring to include `PortSharer`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

mafredri

Other than the open question about groups, LGTM 👍🏻

mafredri · 2026-06-05T08:38:58Z

+					slog.F("max_port_share_level", template.MaxPortSharingLevel),
+					slog.Error(err))
+				sharingLevel = template.MaxPortSharingLevel
+			}


It would be nice to surface this as a warning in the response / on the workspace agent side. But that's definitely not in scope, maybe a future improvement/refactor.

100% agreed, unfortunately we don't seem to have an easy way to surface this kind of things to the user.

johnstcn added 5 commits June 4, 2026 10:34

fix: check template port sharing level in SubAgentAPI

311f744

clamp instead of error

d72158d

refactor new tests to use mocks

e51db6d

add organization share level coverage

1294e09

add e2e-ish test

6274499

johnstcn self-assigned this Jun 4, 2026

Merge branch 'main' into cj/codagt-479

49e0119

johnstcn commented Jun 4, 2026

View reviewed changes

coder-agents-review Bot reviewed Jun 4, 2026

View reviewed changes

address automated code review feedback

4d5a974

coder-agents-review Bot approved these changes Jun 4, 2026

View reviewed changes

Comment thread coderd/agentapi/subagent.go Outdated

Comment thread coderd/agentapi/subagent.go Outdated

Comment thread enterprise/coderd/subagent_test.go Outdated

johnstcn added 2 commits June 4, 2026 16:18

final nits

14c2ffb

Merge branch 'main' into cj/codagt-479

7113ddf

johnstcn marked this pull request as ready for review June 4, 2026 15:19

Copilot AI review requested due to automatic review settings June 4, 2026 15:19

Copilot started reviewing on behalf of johnstcn June 4, 2026 15:19 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread coderd/agentapi/subagent.go

Comment thread enterprise/coderd/subagent_test.go

johnstcn requested review from DanielleMaywood, jdomeracki-coder and mafredri June 4, 2026 15:47

johnstcn added 3 commits June 4, 2026 16:35

also check template group acl

95df4d9

Merge remote-tracking branch 'origin/cj/codagt-479' into cj/codagt-479

03da607

Merge branch 'main' into cj/codagt-479

e473519

Emyrk reviewed Jun 4, 2026

View reviewed changes

Comment thread coderd/agentapi/subagent.go Outdated

mafredri approved these changes Jun 5, 2026

View reviewed changes

johnstcn added 2 commits June 5, 2026 14:29

add org template read to subjectSubagentAPI

f47731f

make enterprise tests more enterprisey

7f27810

johnstcn mentioned this pull request Jun 5, 2026

chore: add test to document current behaviour on template ACL revocation #26104

Merged

johnstcn merged commit 63cd8a8 into main Jun 5, 2026
28 checks passed

johnstcn deleted the cj/codagt-479 branch June 5, 2026 15:30

github-actions Bot locked and limited conversation to collaborators Jun 5, 2026

Uh oh!

Conversation

johnstcn commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear-code Bot commented Jun 4, 2026

Uh oh!

johnstcn commented Jun 4, 2026

Uh oh!

coder-agents-review Bot commented Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Findings

Round log

Round 1

Round 2

Uh oh!

johnstcn Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

johnstcn commented Jun 4, 2026

Uh oh!

johnstcn commented Jun 4, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafredri Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

johnstcn Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

johnstcn commented Jun 4, 2026 •

edited

Loading

coder-agents-review Bot commented Jun 4, 2026 •

edited

Loading