Commit be6654a
authored
🤖 fix: prevent MCP schema panic in control plane status tool (#58)
## Summary
Fix MCP startup panic by replacing `metav1.Condition` in
`get_control_plane_status` tool output with a JSON-schema-safe summary
type, add a regression test to ensure `NewServer` does not panic during
tool registration, and align local lint tooling with CI by using
vendored golangci-lint v2 via `go tool`.
## Background
`coder-k8s` could panic on startup when MCP registered
`get_control_plane_status` because the output schema exposed
`metav1.Condition` (which includes `metav1.Time` embedding `time.Time`).
The schema inference path rejects that embedded custom schema shape,
crashing MCP registration and preventing app startup.
## Implementation
- Added `controlPlaneConditionSummary` and changed
`getControlPlaneStatusOutput.Conditions` to
`[]controlPlaneConditionSummary`.
- Added `summarizeMetav1Conditions(...)` helper to convert condition
fields and stringify `LastTransitionTime` as RFC3339Nano.
- Updated `get_control_plane_status` handler to return summarized
conditions.
- Added `internal/app/mcpapp/server_test.go` with
`TestNewServerDoesNotPanic` using fake clients.
- Updated `Makefile` lint target to use vendored golangci-lint v2:
- `go tool golangci-lint run ./...`
- `go tool golangci-lint fmt --diff`
## Validation
- `make verify-vendor`
- `make test`
- `make build`
- `make lint`
## Risks
Low to medium. The API shape for a single MCP tool response changed from
raw Kubernetes condition objects to summarized condition objects, but
this is intentional to prevent startup panics and is scoped to MCP
output formatting.
---
<details>
<summary>📋 Implementation Plan</summary>
# Fix MCP tool schema panic (`get_control_plane_status`)
## Context / Why
Running `coder-k8s` in `--app=all` (the default in
`deploy/deployment.yaml`) currently crash-loops with a panic during MCP
tool registration:
```
panic: AddTool: tool "get_control_plane_status": output schema: ForType(mcpapp.getControlPlaneStatusOutput): computing element schema: custom schema for embedded struct must have type "object", got "string"
```
This prevents the operator stack from starting at all (controller +
aggregated API server + MCP HTTP run in the same pod).
## Evidence
- `internal/app/mcpapp/tools.go:getControlPlaneStatusOutput` currently
returns `[]metav1.Condition` in the tool output.
- `metav1.Condition` contains `metav1.Time` fields, and **`metav1.Time`
embeds `time.Time`**.
- The vendored schema generator used by MCP
(`vendor/github.com/google/jsonschema-go/jsonschema`) has a hard-coded
custom schema for `time.Time` with `Type: "string"` and rejects
*embedded* structs whose custom schema isn’t `Type: "object"`.
- The failure occurs in
`vendor/github.com/google/jsonschema-go/jsonschema/infer.go` around the
embedded-field check:
```go
if field.Anonymous {
override := schemas[field.Type]
if override != nil {
if override.Type != "object" {
return nil, fmt.Errorf(
`custom schema for embedded struct must have type "object", got %q`,
override.Type,
)
}
}
}
```
- Scan of `internal/app/mcpapp/**/*.go` confirms
`getControlPlaneStatusOutput` is the **only** MCP tool output type that
exposes `metav1.Condition` directly; all other tools already use
“summary” structs with stringified timestamps.
## Implementation plan
### 1) Replace `metav1.Condition` in MCP output with a summary type
**File:** `internal/app/mcpapp/tools.go`
1. Introduce a safe condition summary struct that does **not** include
any Kubernetes time types:
```go
// NOTE: We cannot return metav1.Condition directly because metav1.Time embeds time.Time,
// which causes jsonschema-go schema inference to fail for MCP tool schemas.
type controlPlaneConditionSummary struct {
Type string `json:"type"`
Status string `json:"status"`
Reason string `json:"reason,omitempty"`
Message string `json:"message,omitempty"`
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
LastTransitionTime string `json:"lastTransitionTime,omitempty"`
}
```
2. Update `getControlPlaneStatusOutput` to use this summary type:
```go
type getControlPlaneStatusOutput struct {
ObservedGeneration int64 `json:"observedGeneration"`
ReadyReplicas int32 `json:"readyReplicas"`
URL string `json:"url"`
Phase string `json:"phase"`
Conditions []controlPlaneConditionSummary `json:"conditions"`
}
```
### 2) Convert conditions when handling the tool call
**File:** `internal/app/mcpapp/tools.go`
Add a small helper to keep tool handlers clean and ensure consistent
formatting:
```go
func summarizeMetav1Conditions(in []metav1.Condition) []controlPlaneConditionSummary {
out := make([]controlPlaneConditionSummary, 0, len(in))
for _, cond := range in {
summary := controlPlaneConditionSummary{
Type: cond.Type,
Status: string(cond.Status),
Reason: cond.Reason,
Message: cond.Message,
ObservedGeneration: cond.ObservedGeneration,
}
if !cond.LastTransitionTime.IsZero() {
summary.LastTransitionTime = cond.LastTransitionTime.Time.UTC().Format(time.RFC3339Nano)
}
out = append(out, summary)
}
return out
}
```
Then update the `get_control_plane_status` tool handler to return:
```go
Conditions: summarizeMetav1Conditions(controlPlane.Status.Conditions),
```
### 3) Add a regression test to prevent future panics
**File:** `internal/app/mcpapp/server_test.go` (new)
Goal: ensure that tool registration (schema inference) does not panic.
Test shape:
- Build a fake controller-runtime client with `newScheme()`.
- Use `k8s.io/client-go/kubernetes/fake.NewSimpleClientset()`.
- Call `NewServer(k8sClient, clientset)` inside a `defer recover()`
block and fail the test if it panics.
This catches:
- reintroducing `metav1.Condition` / `metav1.Time` / other embedded-time
types into MCP tool IO structs
- any future schema inference regressions
### 4) Validation / smoke checks
Run locally:
- `make test`
- `make lint` (or at minimum `GOFLAGS=-mod=vendor golangci-lint run` if
that’s the repo norm)
Optional runtime smoke:
- `GOFLAGS=-mod=vendor go run . --app=mcp-http` (should start without
panic)
- Rebuild and load the image into kind and confirm the operator pod
becomes `Ready` when running `--app=all`.
## Acceptance criteria
- `coder-k8s` no longer panics on startup when running `--app=all`.
- MCP HTTP mode starts successfully, and the `get_control_plane_status`
tool returns conditions (in summarized form).
- `go test ./...` (via `make test`) passes.
</details>
---
_Generated with [`mux`](https://github.com/coder/mux) • Model:
`openai:gpt-5.3-codex` • Thinking: `xhigh`_
_Generated with `mux` • Model: `openai:gpt-5.3-codex` • Thinking:
`xhigh` • Cost: `$1.13`_
<!-- mux-attribution: model=openai:gpt-5.3-codex thinking=xhigh
costs=1.13 -->1 parent f9a37bc commit be6654a
3 files changed
Lines changed: 72 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
27 | 27 | | |
28 | 28 | | |
29 | 29 | | |
30 | | - | |
31 | | - | |
32 | | - | |
| 30 | + | |
| 31 | + | |
33 | 32 | | |
34 | 33 | | |
35 | 34 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
50 | 50 | | |
51 | 51 | | |
52 | 52 | | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
53 | 65 | | |
54 | | - | |
55 | | - | |
56 | | - | |
57 | | - | |
58 | | - | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
59 | 71 | | |
60 | 72 | | |
61 | 73 | | |
| |||
293 | 305 | | |
294 | 306 | | |
295 | 307 | | |
296 | | - | |
| 308 | + | |
297 | 309 | | |
298 | 310 | | |
299 | 311 | | |
| |||
873 | 885 | | |
874 | 886 | | |
875 | 887 | | |
| 888 | + | |
| 889 | + | |
| 890 | + | |
| 891 | + | |
| 892 | + | |
| 893 | + | |
| 894 | + | |
| 895 | + | |
| 896 | + | |
| 897 | + | |
| 898 | + | |
| 899 | + | |
| 900 | + | |
| 901 | + | |
| 902 | + | |
| 903 | + | |
| 904 | + | |
| 905 | + | |
| 906 | + | |
| 907 | + | |
| 908 | + | |
| 909 | + | |
| 910 | + | |
876 | 911 | | |
877 | 912 | | |
878 | 913 | | |
| |||
0 commit comments