Deep Review: 20260327-172608-pr-198

Date	2026-03-27 17:26
Repo	rancher-sandbox/rancher-desktop-daemon
Round	1
Author	@mook-as
PR	#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 (important, regression) Claude Codex Gemini ¶

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 BATS test passes because curl does not send an Origin header.

pkg/controllers/mock/logs_passthrough.go

	if err != nil {
		log.V(5).Info("Failed to get container", "error", err)
		http.Error(w, "Failed to get container", http.StatusNotFound)
		return
	}

	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 existing demo controller at pkg/controllers/app/demo/controller.go:90-91 already overrides CheckOrigin for the same reason:

pkg/controllers/app/demo/controller.go

func (c *controller) handleWebSocket(w http.ResponseWriter, r *http.Request) {
	log := ctrl.LoggerFrom(r.Context())
	upgrader := &websocket.Upgrader{
		CheckOrigin: func(_ *http.Request) bool { return true },
	}
	conn, err := upgrader.Upgrade(w, r, nil)

Fix:

-	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 (important, regression) Gemini ¶

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.

bats/tests/34-containers-controllers/containers-mock.bats

    # Grab a random container and check its logs.
    run -0 cat "${TEST_DATA_PATH}/containers.json"
    container_data=${output}
    jq_raw '.[0].Id' "${container_data}"
    container=${output}
    label=org.opencontainers.image.source
    jq_raw ".[0].Config.Labels[\"${label}\"]" "${container_data}"
    value=${output}

    # Set up curl to fetch logs.

Fix:

-    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" (suggestion, regression) Claude Codex ¶

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

pkg/controllers/mock/logs_passthrough.go

var containerIDValidator = regexp.MustCompile(`^[0-9a-fA-F]+$`)

// handleLogs is the handler for the `/passthrough/mocks/logs/{container}`
// endpoint.  It exists for providing fake logs for testing and screenshots.
func (c *controller) handleLogs(w http.ResponseWriter, r *http.Request) {

Fix:

-// 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 (suggestion, enhancement) Gemini ¶

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.

pkg/controllers/mock/logs_passthrough.go

	// We don't have any logs for the mock controller; just print out some
	// stuff to show that we found the container.
	if err := write(fmt.Sprintf("Logs for container %s\n", containerID)); err != nil {
		return
	}
	for k, v := range container.Status.Labels {
		if err := write(fmt.Sprintf("Label: %s\t%s\n", k, v)); err != nil {
			return
		}
	}
}

Fix:

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 — 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
Clean separation: the map + interface adapter at mock_controller.go:52-57 makes adding future endpoints trivial — just add another map entry. Claude
Good input validation: the hex regex at line 22 of logs_passthrough.go prevents path traversal or injection through the container ID parameter. Claude
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
Extracting curl_has_websocket_support into a shared helper removes duplicate regex logic across test files. Gemini

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:

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

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.