# Deep Review: 20260327-172608-pr-198

| | |
|---|---|
| **Date** | 2026-03-27 17:26 |
| **Repo** | [rancher-sandbox/rancher-desktop-daemon](https://github.com/rancher-sandbox/rancher-desktop-daemon) |
| **Round** | 1 |
| **Author** | [@mook-as](https://github.com/mook-as) |
| **PR** | [#198](https://github.com/rancher-sandbox/rancher-desktop-daemon/pull/198) — mock controller: Add a mock logs endpoint |
| **Branch** | `mook-as/container-api/mock-logs` |
| **Commits** | `51e70d5` mock controller: Add a mock logs endpoint |
| **Reviewers** | Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro |
| **Verdict** | **Merge with fixes** — two important issues: WebSocket origin check rejects browser clients, and BATS test calls `jq_raw` without `run -0` |
| **Wall-clock time** | `14 min 43 s` |

---

## Executive Summary

This PR adds a mock WebSocket endpoint (`/passthrough/mock/logs/{container}`) to the mock controller, enabling UI development against fake container log data. It also extracts a shared `curl_has_websocket_support` helper and adds integration tests. The implementation follows established passthrough patterns, but the WebSocket upgrader's default origin check will reject browser clients — the exact use case the endpoint targets.

---

## Critical Issues

None.

---

## Important Issues

1. **WebSocket upgrader rejects cross-origin browser requests** — `pkg/controllers/mock/logs_passthrough.go:48-49` (important, regression) [Claude Opus 4.6, Codex GPT 5.4, Gemini 3.1 Pro]

```go
upgrader := websocket.Upgrader{}
conn, err := upgrader.Upgrade(w, r, nil)
```

Gorilla WebSocket's default `CheckOrigin` rejects requests where the `Origin` header host differs from the `Host` header. The PR description states this endpoint exists "for working on the UI," yet browser clients connecting from a different origin (e.g., webpack dev server on `localhost:3000`, Electron's `app://`) will fail the handshake. The existing demo controller at `pkg/controllers/app/demo/controller.go:90-91` already overrides `CheckOrigin` for the same reason. The BATS test passes because `curl` does not send an `Origin` header.

Fix:

```diff
-	upgrader := websocket.Upgrader{}
+	upgrader := websocket.Upgrader{
+		CheckOrigin: func(_ *http.Request) bool { return true },
+	}
 	conn, err := upgrader.Upgrade(w, r, nil)
```

2. **BATS test calls `jq_raw` without `run -0`** — `bats/tests/34-containers-controllers/containers-mock.bats:56-60` (important, regression) [Gemini 3.1 Pro]

```bash
    jq_raw '.[0].Id' "${container_data}"
    container=${output}
    label=org.opencontainers.image.source
    jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
    value=${output}
```

The test calls `jq_raw` directly instead of wrapping it in `run -0`. This works by accident because `jq_raw` internally calls `run jq ...`, which sets `$output` as a side effect. However, `jq_raw` also echoes to stdout (line 139 of `bats/helpers/utils.bash`), leaking output into the test runner. Every other `jq_raw`/`jq_output` call in this file uses `run -0`. This violates the repo's BATS convention documented in `deep-review-context.md`.

Fix:

```diff
-    jq_raw '.[0].Id' "${container_data}"
+    run -0 jq_raw '.[0].Id' "${container_data}"
     container=${output}
     label=org.opencontainers.image.source
-    jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
+    run -0 jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
     value=${output}
```

---

## Suggestions

1. **Docstring path says "mocks" instead of "mock"** — `pkg/controllers/mock/logs_passthrough.go:24` (suggestion, regression) [Claude Opus 4.6, Codex GPT 5.4]

```go
// handleLogs is the handler for the `/passthrough/mocks/logs/{container}`
// endpoint.  It exists for providing fake logs for testing and screenshots.
```

The controller name (returned by `GetName()` at `mock_controller.go:66`) is `"mock"`, so the actual route is `/passthrough/mock/logs/{container}`.

Fix:

```diff
-// handleLogs is the handler for the `/passthrough/mocks/logs/{container}`
+// handleLogs is the handler for the `/passthrough/mock/logs/{container}`
```

2. **Non-deterministic map iteration produces unstable log output** — `pkg/controllers/mock/logs_passthrough.go:74-78` (suggestion, enhancement) [Gemini 3.1 Pro]

```go
for k, v := range container.Status.Labels {
    if err := write(fmt.Sprintf("Label: %s\t%s\n", k, v)); err != nil {
        return
    }
}
```

Go randomizes map iteration order. Since this endpoint exists for "testing and screenshots," non-deterministic label ordering could produce flaky snapshots or brittle assertions. The current BATS test uses `assert_line` (order-independent), so this is not a problem today, but sorting the keys would prevent surprises.

Fix:

```go
keys := slices.Collect(maps.Keys(container.Status.Labels))
slices.Sort(keys)
for _, k := range keys {
    if err := write(fmt.Sprintf("Label: %s\t%s\n", k, container.Status.Labels[k])); err != nil {
        return
    }
}
```

---

## Design Observations

**Concerns**

- **WebSocket upgrade policy should be centralized** (future) [Codex GPT 5.4] — The mock handler reimplements WebSocket upgrade/close logic instead of sharing the pattern in the demo controller. This divergence caused Finding 1. A small helper in `pkg/controllers/base` for "upgrade a passthrough WebSocket with the project-default origin policy" would prevent this class of bug from recurring.

**Strengths**

- The passthrough wires through the existing discovery/ConfigMap mechanism instead of special-case routing, keeping the service proxy model consistent. [Codex GPT 5.4]
- Clean separation: the map + interface adapter at `mock_controller.go:52-57` makes adding future endpoints trivial — just add another map entry. [Claude Opus 4.6]
- Good input validation: the hex regex at line 22 of `logs_passthrough.go` prevents path traversal or injection through the container ID parameter. [Claude Opus 4.6]
- The WebSocket close sequence (deferred close message then `conn.Close()`, LIFO order) at lines 54-61 is more robust than the demo controller's bare `defer conn.Close()`. [Claude Opus 4.6]
- Extracting `curl_has_websocket_support` into a shared helper removes duplicate regex logic across test files. [Gemini 3.1 Pro]

---

## Testing Assessment

Integration tests verify the passthrough registration via the discovery ConfigMap and WebSocket connectivity with actual data output. The `curl_has_websocket_support` check correctly skips on systems without WebSocket curl support.

Untested scenarios, ranked by risk:

1. Browser-style WebSocket connections with an `Origin` header (validates Finding 1).
2. Negative paths: invalid container ID (400) and missing container (404) at `logs_passthrough.go:31-45`. [Codex GPT 5.4]

---

## Documentation Assessment

The docstring path mismatch is covered by Suggestion 1. No other documentation gaps identified.

---

## Agent Performance Retro

### [Claude]

Claude produced a thorough review with detailed tracing into the demo controller and passthrough proxy to verify the CheckOrigin finding. It identified the docstring typo and provided the richest design observations (input validation, close sequence). However, it marked `containers-mock.bats` as "Reviewed, no issues" — missing the `jq_raw` convention violation that Gemini caught.

### [Codex]

Codex was the fastest agent (165s) and produced a focused review. It correctly identified the CheckOrigin issue with a clear explanation of why `curl` masks the problem. It raised the valuable design concern about centralizing WebSocket upgrade policy. It noted the docstring mismatch in its Documentation Assessment but did not elevate it to a finding. Like Claude, it marked `containers-mock.bats` as "Reviewed, no issues."

### [Gemini]

Gemini provided the broadest coverage. It was the only agent to catch the `jq_raw` without `run -0` convention violation — a real issue both other agents missed. It also raised the map iteration order concern as a suggestion. Its CheckOrigin analysis was thorough and well-sourced.

### Summary

| Metric | Claude Opus 4.6 | Codex GPT 5.4 | Gemini 3.1 Pro |
|--------|-----------------|----------------|----------------|
| Duration | 5:57 | 2:45 | 4:41 |
| Critical | 0 | 0 | 0 |
| Important | 1 | 1 | 2 |
| Suggestion | 1 | 0 | 1 |
| Design observations | 4 | 2 | 1 |
| False positives | 0 | 0 | 0 |
| Unique insights | 0 | 1 (centralize WS policy) | 2 (jq_raw, map order) |
| Files reviewed | 5/5 | 5/5 | 5/5 |
| Coverage misses | 1 (containers-mock.bats) | 1 (containers-mock.bats) | 0 |

Gemini delivered the most value this round, catching a convention violation both other agents overlooked. Codex was fastest with the most actionable design insight. Claude provided the deepest context tracing but had a blind spot on BATS conventions.

---

## Appendix: Original Reviews

### Claude Opus 4.6

Now I have all the context I need. Let me compile the review.

---

## Executive Summary

This PR adds a mock WebSocket logs endpoint (`/passthrough/mock/logs/{containerID}`) to the mock controller, enabling UI development against fake container log data. The implementation is clean and follows established passthrough patterns. The main concern is a missing `CheckOrigin` override on the WebSocket upgrader that will reject browser requests from different origins during UI development, and a minor docstring typo.

**Verdict: Approve with minor changes**

## Findings

### Critical Issues

None.

### Important Issues

1. **Missing `CheckOrigin` on WebSocket Upgrader will reject cross-origin browser requests** — `pkg/controllers/mock/logs_passthrough.go:48` (important, regression)

```go
upgrader := websocket.Upgrader{}
conn, err := upgrader.Upgrade(w, r, nil)
```

The default gorilla/websocket `CheckOrigin` rejects requests where the `Origin` header host does not match the `Host` header. The existing demo controller WebSocket handler at `pkg/controllers/app/demo/controller.go:90-91` explicitly overrides this:

```go
upgrader := &websocket.Upgrader{
    CheckOrigin: func(_ *http.Request) bool { return true },
}
```

The passthrough reverse proxy at `pkg/service/controllers/passthrough.go:58` uses `httputil.NewSingleHostReverseProxy`, which preserves the original `req.Host` header. So for same-origin browser requests, `CheckOrigin` passes. However, during UI development it's common to serve the frontend from a different origin (e.g., webpack dev server on `localhost:3000` while the API is on `localhost:6443`). In that case, `Origin: http://localhost:3000` vs `Host: localhost:6443` → CheckOrigin rejects the connection silently. Since the PR description states this is for UI development, this will be a real problem.

`curl` does not send an `Origin` header for WebSocket requests, so the BATS test passes — but browser clients will fail in cross-origin development setups.

Fix:
```diff
-	upgrader := websocket.Upgrader{}
+	upgrader := websocket.Upgrader{
+		CheckOrigin: func(_ *http.Request) bool { return true },
+	}
```

This matches the demo controller's pattern and is appropriate for an internal-only, single-user API server.

### Suggestions

1. **Docstring says "mocks" but the controller name is "mock"** — `pkg/controllers/mock/logs_passthrough.go:24` (suggestion, regression)

```go
// handleLogs is the handler for the `/passthrough/mocks/logs/{container}`
// endpoint.  It exists for providing fake logs for testing and screenshots.
```

The controller name (returned by `GetName()` at line 66 of `mock_controller.go`) is `"mock"`, so the actual path is `/passthrough/mock/logs/{container}`.

Fix:
```diff
-// handleLogs is the handler for the `/passthrough/mocks/logs/{container}`
+// handleLogs is the handler for the `/passthrough/mock/logs/{container}`
```

## Design Observations

**Strengths**

- Clean separation: the passthrough registration via the map + interface adapter at `mock_controller.go:52-57` makes adding future endpoints trivial — just add another map entry.
- Good input validation: the hex regex at line 22 prevents path traversal or injection through the container ID parameter.
- The WebSocket close sequence (deferred close message then `conn.Close()`, LIFO order) at lines 54-61 is more robust than the demo controller's bare `defer conn.Close()`.
- Extracting `curl_has_websocket_support` into a shared helper avoids duplicating the protocol detection logic across test files.

## Testing Assessment

Test coverage is adequate for a mock endpoint:

1. The test verifies passthrough registration via the discovery ConfigMap (lines 44-47).
2. It verifies WebSocket connectivity and actual data output (lines 67-70).
3. The `curl_has_websocket_support` check correctly skips on systems without WebSocket curl support.

No untested scenarios of significant risk identified.

## Coverage Summary

| File | Status |
|------|--------|
| `bats/helpers/commands.bash` | **Reviewed, no issues** — `curl_has_websocket_support` correctly extracted from passthrough.bats; regex `\ wss?` is functionally equivalent to the original `\ ws` pattern. |
| `bats/tests/32-app-controllers/passthrough.bats` | **Reviewed, no issues** — SPDX headers added; WebSocket check refactored to use the new shared helper. |
| `bats/tests/34-containers-controllers/containers-mock.bats` | **Reviewed, no issues** — SPDX headers added; new test follows established patterns (jq_raw for extraction, assert_line for validation, proper WebSocket skip logic). |
| `pkg/controllers/mock/logs_passthrough.go` | **Finding 1, Finding S1** — CheckOrigin gap and docstring typo. |
| `pkg/controllers/mock/mock_controller.go` | **Reviewed, no issues** — PassthroughController implementation is clean; `c.mgr = mgr` stored at registration (line 173) is available before the handler is called since the passthrough server starts after registration. |

### Codex GPT 5.4

**Verdict**: Changes requested

### Executive Summary

This PR adds a mock `logs` passthrough endpoint to the mock controller and extends BATS coverage so the endpoint can be exercised through the service proxy. The implementation is close, but the new WebSocket handler uses Gorilla's default origin policy, which will reject the browser-originated connections this endpoint is being added for.

### Findings

#### Critical Issues

None.

#### Important Issues

1. **Mock logs WebSocket rejects browser/UI clients by default** — [`pkg/controllers/mock/logs_passthrough.go:48-49`](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/mock/logs_passthrough.go#L48) (important, regression)

```go
upgrader := websocket.Upgrader{}
conn, err := upgrader.Upgrade(w, r, nil)
if err != nil {
	log.V(5).Info("Failed to upgrade to WebSocket", "error", err)
	return
}
```

The new handler creates a plain `websocket.Upgrader{}` at line 48 and upgrades at line 49. In Gorilla WebSocket, that means the default origin check is active, so any request with an `Origin` header that does not match the proxied target host is rejected. That matters here because the PR description says this endpoint exists "for working on the UI", and the existing demo passthrough explicitly disables origin filtering in [`pkg/controllers/app/demo/controller.go:90-91`](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/app/demo/controller.go#L90) for exactly this browser-facing use case. As written, a browser/Electron frontend connecting through `/passthrough/mock/logs/{id}` will fail the handshake even though the curl-based BATS test at lines 67-70 in [`bats/tests/34-containers-controllers/containers-mock.bats`](/Users/jan/git/rancher-desktop-daemon/bats/tests/34-containers-controllers/containers-mock.bats#L67) passes, because curl does not send an `Origin` header.

Fix: configure the upgrader with an explicit `CheckOrigin` policy consistent with the existing passthroughs, or better, factor WebSocket upgrade policy into a shared helper so all passthrough endpoints use the same origin behavior.

```diff
-	upgrader := websocket.Upgrader{}
+	upgrader := websocket.Upgrader{
+		CheckOrigin: func(_ *http.Request) bool { return true },
+	}
 	conn, err := upgrader.Upgrade(w, r, nil)
```

#### Suggestions

None.

### Design Observations

**Concerns**

1. Centralizing WebSocket upgrade policy would reduce drift across passthrough controllers `(future)`. The mock handler reimplements upgrade/close behavior instead of sharing the pattern already present in the demo controller, which is how this origin-policy mismatch slipped in. A small helper in `pkg/controllers/base` for "upgrade a passthrough WebSocket with the project-default policy" would eliminate a repeat class of bugs.

**Strengths**

1. The passthrough is wired through the existing discovery/configmap mechanism instead of adding special-case routing. That keeps the service proxy model consistent with the rest of the controller framework.

### Testing Assessment

1. Browser-style WebSocket connections with an `Origin` header are not covered. The new BATS test uses curl, so it misses the failing case introduced at [`pkg/controllers/mock/logs_passthrough.go:48-49`](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/mock/logs_passthrough.go#L48).
2. The negative paths for invalid container IDs and missing containers are untested. There is no coverage for the `400` returned at lines 31-34 or the `404` returned at lines 42-45 in [`pkg/controllers/mock/logs_passthrough.go`](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/mock/logs_passthrough.go#L31).

`go test ./pkg/service/controllers/... ./pkg/controllers/mock/... ./pkg/controllers/app/demo/...` passed locally.

### Documentation Assessment

No external docs look necessary for this mock-only endpoint. The inline comment at [`pkg/controllers/mock/logs_passthrough.go:24`](/Users/jan/git/rancher-desktop-daemon/pkg/controllers/mock/logs_passthrough.go#L24) says `/passthrough/mocks/logs/{container}`, but the actual route is `/passthrough/mock/logs/{container}`.

### Commit Structure

The PR scope is clean: one commit, one concept, and the message matches the diff.

### Coverage Summary

- `bats/helpers/commands.bash` — Reviewed, no issues
- `bats/tests/32-app-controllers/passthrough.bats` — Reviewed, no issues
- `bats/tests/34-containers-controllers/containers-mock.bats` — Reviewed, no issues
- `pkg/controllers/mock/logs_passthrough.go` — Finding 1
- `pkg/controllers/mock/mock_controller.go` — Reviewed, no issues

### Gemini 3.1 Pro

### Executive Summary

This change introduces a new HTTP passthrough endpoint `/passthrough/mock/logs/{container}` to the mock controller, providing a mocked WebSocket stream of container logs for UI testing and screenshots. It also refactors a `curl` WebSocket support check into a shared BATS helper and adds integration tests. The implementation correctly hooks into the passthrough infrastructure, but requires a `CheckOrigin` adjustment to work securely with cross-origin UI requests, and has a BATS testing standard violation.

### Findings

#### Critical Issues

None.

#### Important Issues

1. **WebSocket upgrade will reject cross-origin requests from the UI** — `pkg/controllers/mock/logs_passthrough.go:48-49` (important, regression)

```go
	upgrader := websocket.Upgrader{}
	conn, err := upgrader.Upgrade(w, r, nil)
```

The PR description notes this endpoint is meant to be used "for the front end later." By default, Gorilla WebSocket's `Upgrader` [enforces strict origin checking](https://pkg.go.dev/github.com/gorilla/websocket#hdr-Origin_Considerations), failing the upgrade with a 403 Forbidden if the `Origin` request header does not exactly match the `Host` header. Because the Rancher Desktop UI typically runs on a different protocol/port (e.g., `app://` or a local dev server), it will be blocked from connecting to this endpoint unless an explicit override is provided.

Fix: Allow all origins for this mock endpoint, matching the pattern already used in the `demo` controller.

```diff
-	upgrader := websocket.Upgrader{}
+	upgrader := websocket.Upgrader{
+		CheckOrigin: func(_ *http.Request) bool { return true },
+	}
 	conn, err := upgrader.Upgrade(w, r, nil)
```

2. **Missing `run -0` for `jq_raw` in BATS tests** — `bats/tests/34-containers-controllers/containers-mock.bats:56-60` (important, regression)

```bash
    jq_raw '.[0].Id' "${container_data}"
    container=${output}
    label=org.opencontainers.image.source
    jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
    value=${output}
```

The test invokes `jq_raw` without wrapping it in `run -0`. This violates the repository's BATS style guidelines and relies on a fragile side effect where `jq_raw`'s internal `run` call leaks its `$output` variable to the caller. Furthermore, without `run`, `jq_raw` prints to stdout, which leaks into the test runner's raw output instead of being captured.

Fix: Capture the output explicitly with `run -0`.

```diff
-    jq_raw '.[0].Id' "${container_data}"
+    run -0 jq_raw '.[0].Id' "${container_data}"
     container=${output}
     label=org.opencontainers.image.source
-    jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
+    run -0 jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
     value=${output}
```

#### Suggestions

1. **Non-deterministic map iteration leads to unstable mock logs** — `pkg/controllers/mock/logs_passthrough.go:74-78` (suggestion, enhancement)

```go
	for k, v := range container.Status.Labels {
		if err := write(fmt.Sprintf("Label: %s\t%s\n", k, v)); err != nil {
			return
		}
	}
```

In Go, map iteration order is randomized. Because this endpoint exists to provide "fake logs for testing and screenshots," returning the label lines in a random order on every request may cause flaky snapshots or brittle end-to-end test assertions in the future.

Fix: Sort the map keys before iterating to guarantee a stable output order.

```go
	keys := slices.Collect(maps.Keys(container.Status.Labels))
	slices.Sort(keys)
	for _, k := range keys {
		if err := write(fmt.Sprintf("Label: %s\t%s\n", k, container.Status.Labels[k])); err != nil {
			return
		}
	}
```

### Design Observations

**Strengths**

The extraction of `curl_has_websocket_support` in `bats/helpers/commands.bash` is a clean refactor that removes duplicate regex logic across multiple tests and makes the WebSocket requirement explicit.

### Testing Assessment

Integration tests adequately verify that the logs passthrough endpoint successfully authenticates, finds the mock container, upgrades to WebSocket, and streams the expected log messages. 

### Documentation Assessment

No major documentation gaps. 

### Coverage Summary

- `bats/helpers/commands.bash` — Reviewed, no issues
- `bats/tests/32-app-controllers/passthrough.bats` — Reviewed, no issues
- `bats/tests/34-containers-controllers/containers-mock.bats` — Finding 2
- `pkg/controllers/mock/logs_passthrough.go` — Findings 1, 3
- `pkg/controllers/mock/mock_controller.go` — Reviewed, no issues