PLAT-378

• chore: forbid direct response body JSON decode in codersdk #27859 👈 (View in Graphite)
• refactor(codersdk): use shared error helpers in chat endpoints #27858
• refactor(codersdk): use ReadBodyAsJSON in typed endpoints #27857
• fix: add codersdk JSON response decoder for typed API endpoints #27804
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Review — focused on scripts/rules.go

No blocking issues. The rule is correctly scoped, actually fires, doesn't leak outside codersdk, and CI lint is green. Everything below is a suggestion or a nit.

Because an over-narrow ruleguard pattern degrades silently (a pattern that stops matching is a no-op with green CI), I didn't review the matcher by inspection alone — I built a worktree at ca750d8e3, added probe files, and ran the pinned linter (golangci-lint v1.64.8, matching mise.toml:29).

1. The documented gap is occupied, not hypothetical — worth a follow-up

The rule comment notes that the UseNumber decoders in licenses.go evade it. They do, and they're real control-plane endpoints:

(same shape again at licenses.go:123-126 in Client.Licenses)

coder licenses add and coder licenses list hit /api/v2/licenses through the same reverse proxy or SSO portal as coder whoami. In the exact #27044 scenario those two commands still print invalid character '<' looking for beginning of value. The stack closed the bug for ~280 call sites and left two open — and this PR institutionalises that rather than closing it.

I tested the fix. Adding two patterns to the same m.Match catches precisely those two sites and produces no other findings across ./codersdk/...:

	m.Match(
		`json.NewDecoder($res.Body).Decode($_)`,
		`$_ := json.NewDecoder($res.Body)`,
		`$_ = json.NewDecoder($res.Body)`,
	).

codersdk/licenses.go:109:2: ruleguard: Use codersdk.ReadBodyAsJSON to decode ...
codersdk/licenses.go:124:2: ruleguard: Use codersdk.ReadBodyAsJSON to decode ...

Ordering constraint: the matcher can't land before the call sites are dealt with, or lint goes red. ReadBodyAsJSON has no UseNumber knob, so either:

1. Add a UseNumber-capable variant (options struct, or ReadBodyAsJSONNumbers) and migrate both sites — closes the bug and the rule gap together. Preferred.
2. Land the matcher plus //nolint:gocritic // TODO(#27044): needs UseNumber support in ReadBodyAsJSON. — doesn't fix coder licenses, but stops the two-step form spreading silently and leaves a tracked marker instead of a rule comment nobody will re-read.

Either is fine as a follow-up; I wouldn't hold this PR for it. But "the rule documents its own blind spot" is weaker than it looks when the blind spot is currently occupied.

2. The exemption rationale in agentconn.go contradicts the adjacent line — wording only

The comment reads "Agent-direct HTTP API response, not the Coder control-plane API." But eight of those same functions already use the Coder API error contract on the failure path — ListContainers (agentconn.go:607), RecreateDevcontainer (:898), StartProcess, ListProcesses, ContextConfig, CallMCPTool, ProcessOutput, SignalProcess all call codersdk.ReadBodyAsError(res) immediately above the exempted decode. So the responses are Coder-shaped, and the stated reason doesn't survive a reader checking it against the line two above.

The genuinely durable justifications are different, and stronger:

1. ReadBodyAsJSON's user-facing strings are control-plane specific — "from the Coder API" and "Ensure the Coder URL is correct and that any reverse proxy or SSO in front of it passes /api/v2 requests through to Coder." That advice is actively wrong for a request made over tailnet directly to an agent.
2. Agent-direct requests can't be intercepted by a proxy or SSO portal, so the failure mode ReadBodyAsJSON exists to catch isn't reachable there.

Suggest something like "Agent-direct over tailnet; ReadBodyAsJSON's proxy/SSO error text does not apply." Same exemption, a reason that holds up.

3. //nolint:gocritic suppresses all gocritic checks on those lines — non-blocking

golangci-lint v1 nolint directives are linter-granular; there's no //nolint:gocritic:ruleguard and no way to name an individual ruleguard rule. So each of the 17 annotations also disables badLock, weakCond, nilValReturn, typeAssertChain, sqlQuery, and every future ruleguard rule on that line — and agentconn.go is exactly where a future rule about response handling would want to look.

This matches existing convention (the dbauthz rule is suppressed the same way), so it's an accepted cost rather than a defect. If you want the blast radius at 1 line instead of 17, the agent-direct paths could route through one helper:

// decodeAgentJSON decodes an agent-direct HTTP response. Agent responses are
// not Coder control-plane API responses, so codersdk.ReadBodyAsJSON's
// proxy/SSO-oriented error text would be misleading here.
//nolint:gocritic // See doc comment.
func decodeAgentJSON(res *http.Response, v any) error {
	return json.NewDecoder(res.Body).Decode(v)
}

That also removes 16 repetitions of the same 90-character comment and gives the exemption one place to be revisited. Purely a maintainability call.

4. Nits

• Value-typed http.Response isn't matched. Type.Is("*http.Response") is pointer-exact (verified, case 7). net/http always returns a pointer so this is theoretical; Type.Is("*http.Response") || Type.Is("http.Response") closes it if you care.
• No test for the rule. Consistent with all eleven existing rules, so not out of step — just naming the risk: failOn: all makes a broken rules file fail loudly, but a pattern that silently stops matching (refactor, DSL bump, typo'd metavariable) degrades to a no-op with green CI.
• Contributor gotcha worth a line in the PR description: ruleguard results are cached. I confirmed this — after reverting an experimental edit to rules.go, the old rule kept firing until golangci-lint cache clean. CI is immune (cache key is hashFiles('**/*.go')), but someone pulling this change and seeing no new failures may just have a stale cache.

Summary

LGTM. The one thing I'd chase after merge is #1: coder licenses list behind an SSO portal still produces the invalid character '<' error this whole stack set out to eliminate.

Review — focused on scripts/rules.go

No blocking issues. The rule is correctly scoped, actually fires, doesn't leak outside codersdk, and CI lint is green. Everything below is a suggestion or a nit.

Because an over-narrow ruleguard pattern degrades silently (a pattern that stops matching is a no-op with green CI), I didn't review the matcher by inspection alone — I built a worktree at ca750d8e3, added probe files, and ran the pinned linter (golangci-lint v1.64.8, matching mise.toml:29).

Verification matrix — golangci-lint run --disable-all --enable=gocritic --fix=false ./codersdk/... ./cli/

Form under test Reported? Correct?

1 json.NewDecoder(res.Body).Decode(&v), res *http.Response, pkg codersdk yes ✅
2 same + //nolint:gocritic no ✅ suppression works
3 d := json.NewDecoder(res.Body) … d.Decode(&v) no ⚠️ gap
4 d := json.NewDecoder(res.Body); d.UseNumber(); d.Decode(&v) no ⚠️ gap (live — see below)
5 json.NewDecoder(f().Body).Decode(&v), f returns *http.Response yes ✅ call exprs bind fine
6 json.NewDecoder(req.Body).Decode(&v), req *http.Request no ✅ correctly excluded
7 value receiver res http.Response no ⚠️ nit
8 violation in codersdk/agentsdk yes ✅ subpackages covered
9 violation in codersdk/workspacesdk yes ✅ subpackages covered
10 identical violation in cli no ✅ no scope leak
11 identical violation in codersdk/*_test.go no ✅ test exclusion works
Also confirmed:

• PR as submitted is clean — after golangci-lint cache clean, gocritic over ./codersdk/... exits 0.
• Exemption count reconciles exactly — 17 chained .Body).Decode sites exist in the non-test codersdk tree (16 in workspacesdk/agentconn.go, 1 in agentsdk/azure.go), all 17 annotated, zero unannotated. So lint isn't passing by accident of pre-suppression.
• run.tests is unset in .golangci.yaml and defaults to true, so the _test.go gate is load-bearing rather than decorative.

1. The documented gap is occupied, not hypothetical — worth a follow-up

The rule comment notes that the UseNumber decoders in licenses.go evade it. They do, and they're real control-plane endpoints:

coder/codersdk/licenses.go

Lines 108 to 111 in ca750d8

	var l License
	d := json.NewDecoder(res.Body)
	d.UseNumber()
	return l, d.Decode(&l)

(same shape again at licenses.go:123-126 in Client.Licenses)

coder licenses add and coder licenses list hit /api/v2/licenses through the same reverse proxy or SSO portal as coder whoami. In the exact #27044 scenario those two commands still print invalid character '<' looking for beginning of value. The stack closed the bug for ~280 call sites and left two open — and this PR institutionalises that rather than closing it.

I tested the fix. Adding two patterns to the same m.Match catches precisely those two sites and produces no other findings across ./codersdk/...:

	m.Match(
		`json.NewDecoder($res.Body).Decode($_)`,
		`$_ := json.NewDecoder($res.Body)`,
		`$_ = json.NewDecoder($res.Body)`,
	).

codersdk/licenses.go:109:2: ruleguard: Use codersdk.ReadBodyAsJSON to decode ...
codersdk/licenses.go:124:2: ruleguard: Use codersdk.ReadBodyAsJSON to decode ...

Ordering constraint: the matcher can't land before the call sites are dealt with, or lint goes red. ReadBodyAsJSON has no UseNumber knob, so either:

1. Add a UseNumber-capable variant (options struct, or ReadBodyAsJSONNumbers) and migrate both sites — closes the bug and the rule gap together. Preferred.
2. Land the matcher plus //nolint:gocritic // TODO(#27044): needs UseNumber support in ReadBodyAsJSON. — doesn't fix coder licenses, but stops the two-step form spreading silently and leaves a tracked marker instead of a rule comment nobody will re-read.

Either is fine as a follow-up; I wouldn't hold this PR for it. But "the rule documents its own blind spot" is weaker than it looks when the blind spot is currently occupied.

2. The exemption rationale in agentconn.go contradicts the adjacent line — wording only

The comment reads "Agent-direct HTTP API response, not the Coder control-plane API." But eight of those same functions already use the Coder API error contract on the failure path — ListContainers (agentconn.go:607), RecreateDevcontainer (:898), StartProcess, ListProcesses, ContextConfig, CallMCPTool, ProcessOutput, SignalProcess all call codersdk.ReadBodyAsError(res) immediately above the exempted decode. So the responses are Coder-shaped, and the stated reason doesn't survive a reader checking it against the line two above.

The genuinely durable justifications are different, and stronger:

1. ReadBodyAsJSON's user-facing strings are control-plane specific — "from the Coder API" and "Ensure the Coder URL is correct and that any reverse proxy or SSO in front of it passes /api/v2 requests through to Coder." That advice is actively wrong for a request made over tailnet directly to an agent.
2. Agent-direct requests can't be intercepted by a proxy or SSO portal, so the failure mode ReadBodyAsJSON exists to catch isn't reachable there.

Suggest something like "Agent-direct over tailnet; ReadBodyAsJSON's proxy/SSO error text does not apply." Same exemption, a reason that holds up.

3. //nolint:gocritic suppresses all gocritic checks on those lines — non-blocking

golangci-lint v1 nolint directives are linter-granular; there's no //nolint:gocritic:ruleguard and no way to name an individual ruleguard rule. So each of the 17 annotations also disables badLock, weakCond, nilValReturn, typeAssertChain, sqlQuery, and every future ruleguard rule on that line — and agentconn.go is exactly where a future rule about response handling would want to look.

This matches existing convention (the dbauthz rule is suppressed the same way), so it's an accepted cost rather than a defect. If you want the blast radius at 1 line instead of 17, the agent-direct paths could route through one helper:

// decodeAgentJSON decodes an agent-direct HTTP response. Agent responses are
// not Coder control-plane API responses, so codersdk.ReadBodyAsJSON's
// proxy/SSO-oriented error text would be misleading here.
//nolint:gocritic // See doc comment.
func decodeAgentJSON(res *http.Response, v any) error {
	return json.NewDecoder(res.Body).Decode(v)
}

That also removes 16 repetitions of the same 90-character comment and gives the exemption one place to be revisited. Purely a maintainability call.

4. Nits

• Value-typed http.Response isn't matched. Type.Is("*http.Response") is pointer-exact (verified, case 7). net/http always returns a pointer so this is theoretical; Type.Is("*http.Response") || Type.Is("http.Response") closes it if you care.
• No test for the rule. Consistent with all eleven existing rules, so not out of step — just naming the risk: failOn: all makes a broken rules file fail loudly, but a pattern that silently stops matching (refactor, DSL bump, typo'd metavariable) degrades to a no-op with green CI.
• Contributor gotcha worth a line in the PR description: ruleguard results are cached. I confirmed this — after reverting an experimental edit to rules.go, the old rule kept firing until golangci-lint cache clean. CI is immune (cache key is hashFiles('**/*.go')), but someone pulling this change and seeing no new failures may just have a stale cache.

Summary

Blocking issues none
Rule fires as intended verified empirically, 11-case matrix
Scope correctness verified — subpackages in, cli/coderd out, _test.go out
Exemption count reconciles 17/17 chained sites annotated, 0 unannotated
Follow-up worth filing licenses.go UseNumber sites — same bug class, still open
LGTM. The one thing I'd chase after merge is #1: coder licenses list behind an SSO portal still produces the invalid character '<' error this whole stack set out to eliminate.

All addressed in f48321e:

1: Went with the preferred option: added ReadBodyAsJSONUseNumber, migrated both licenses.go sites, and extended the matcher with your assignment patterns. The rule no longer documents its own blind spot, and coder licenses add/list now get structured errors too.

2 & 3: Solved together — the 16 decodes now route through a decodeAgentJSON helper whose doc comment carries the corrected rationale (tailnet-direct, so the proxy/SSO failure mode isn't reachable). One nolint instead of sixteen, and the contradictory wording is gone.

4: Took the http.Response value-type hardening and put the cache-clean gotcha in the PR description. Skipped rule tests for now since none of the existing eleven have them — happy to file a follow-up if we want that infra.