All three concerns from my previous review have been addressed — two substantively in code, one convincingly in argument.
rpc.gen.go panic comment — The author's point stands: the same generated panic("not implemented") exists in 153 other methods in the repo, and the right place for that documentation is the code generator, not a one-off annotation in this file. I accept that reasoning.
cleanEntityID divergence — The generic local copy is gone. The server now strips "app/" with a single strings.TrimPrefix inline where it's used (line 60 of server.go), which is narrower and honest about what it's doing rather than maintaining a partial reimplementation of ui.CleanEntityID. The divergence risk I raised is resolved at the source.
Status normalization equivalence — This is the cleanest fix of the three. Both the RPC path and the legacy entity path now funnel through newSandboxListEntry, which calls ui.CleanStatus unconditionally. sandboxTerminal only ever sees already-normalized strings. The structural invariant I wanted enforced by convention is now enforced by the code path itself. TestLegacySandboxListDropsExecutionSpec seeds a raw compute_v1alpha.DEAD status and asserts the dead sandbox is hidden by the default filter — that's exactly the regression test I asked for.
The core security guarantee — that sandboxListEntry is an allow-list that can never serialize SandboxSpec, and that the serverPredatesSandboxInventory fallback cannot be triggered by auth or unreachable errors — remains intact and is tested at three independent layers (server_test.go, sandbox_list_test.go, blackbox/sandbox_test.go). This is ready to graduate from draft to human review.
Posted to the PR's comment threads when you submit.
cleanEntityID function is gone entirely. The server now does strings.TrimPrefix(version.App.String(), "app/") inline where it's needed, which is more honest and eliminates the divergence risk. The thread is outdated because the code it pointed at no longer exists — this concern is fully addressed.newSandboxListEntry, which calls ui.CleanStatus before the entry is handed to sandboxTerminal. The invariant is structural now, not just asserted by comment. And TestLegacySandboxListDropsExecutionSpec seeds a raw compute_v1alpha.DEAD and verifies the default filter hides it — that's the regression coverage I asked for. Resolved.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1154,
"verdict": "ready",
"event": "comment",
"summary": "All three concerns from my previous review have been addressed — two substantively in code, one convincingly in argument.\n\n**`rpc.gen.go` panic comment** — The author's point stands: the same generated `panic(\"not implemented\")` exists in 153 other methods in the repo, and the right place for that documentation is the code generator, not a one-off annotation in this file. I accept that reasoning.\n\n**`cleanEntityID` divergence** — The generic local copy is gone. The server now strips `\"app/\"` with a single `strings.TrimPrefix` inline where it's used (line 60 of `server.go`), which is narrower and honest about what it's doing rather than maintaining a partial reimplementation of `ui.CleanEntityID`. The divergence risk I raised is resolved at the source.\n\n**Status normalization equivalence** — This is the cleanest fix of the three. Both the RPC path and the legacy entity path now funnel through `newSandboxListEntry`, which calls `ui.CleanStatus` unconditionally. `sandboxTerminal` only ever sees already-normalized strings. The structural invariant I wanted enforced by convention is now enforced by the code path itself. `TestLegacySandboxListDropsExecutionSpec` seeds a raw `compute_v1alpha.DEAD` status and asserts the dead sandbox is hidden by the default filter — that's exactly the regression test I asked for.\n\nThe core security guarantee — that `sandboxListEntry` is an allow-list that can never serialize `SandboxSpec`, and that the `serverPredatesSandboxInventory` fallback cannot be triggered by auth or unreachable errors — remains intact and is tested at three independent layers (`server_test.go`, `sandbox_list_test.go`, `blackbox/sandbox_test.go`). This is ready to graduate from draft to human review.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6fGusQ",
"body": "Confirmed: the generic `cleanEntityID` function is gone entirely. The server now does `strings.TrimPrefix(version.App.String(), \"app/\")` inline where it's needed, which is more honest and eliminates the divergence risk. The thread is outdated because the code it pointed at no longer exists — this concern is fully addressed.",
"resolve": true,
"resolve_reason": "addressed"
},
{
"thread_id": "PRRT_kwDONZ__ss6fGusT",
"body": "This is exactly what I was after. Both paths now go through `newSandboxListEntry`, which calls `ui.CleanStatus` before the entry is handed to `sandboxTerminal`. The invariant is structural now, not just asserted by comment. And `TestLegacySandboxListDropsExecutionSpec` seeds a raw `compute_v1alpha.DEAD` and verifies the default filter hides it — that's the regression coverage I asked for. Resolved.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true,
"draft": true
}This draft fixes a real security problem: the old sandbox list --format json embedded compute_v1alpha.Sandbox directly into the response struct, which meant the full SandboxSpec — including resolved container environment variables and therefore secrets — could leak to any caller of that command. The fix is architecturally sound: a new servers/sandbox package acts as the explicit security boundary, a dedicated SandboxInfo RPC type carries only the safe inventory fields, the CLI renders from that allow-list, and tests assert (with canary values) that the secret-bearing data never makes it to the wire. That's the right shape for this kind of fix.
A few things I want to flag before this graduates to merge:
The reexportSandboxes.List panic is intentional but undocumented. rpc.gen.go line 330–332 panic("not implemented"). The generated Export() path is a code-gen pattern I'd expect to exist for completeness, and the Export() method itself isn't called anywhere in the current codebase. But a panic in a generated exported function with no comment explaining why it panics or what it's for is a landmine for the next person reading this. A // re-export is a no-op for generated clients; not called server-side comment would cost nothing.
sandboxTerminal is inconsistent with the legacy path. In listSandboxesFromEntities, the status is produced by ui.CleanStatus(string(sb.Status)). The sandboxTerminal function checks for "stopped" || "dead". In the new RPC server path (server.go line 120), the server strips the "status." prefix itself with strings.TrimPrefix. Both paths should produce normalized strings, so this should be fine in practice — but the assumption depends on ui.CleanStatus and strings.TrimPrefix("status.", …) being equivalent across all status values. There's no test asserting this equivalence, and if a new status value is added with a different prefix format, the dead-hiding logic could silently break for one path but not the other.
The fallback discriminator is tight and correct, but its narrowness is the whole game. serverPredatesSandboxInventory returns true only for rpc.ErrResolveLookup. The unit test (TestServerPredatesSandboxInventoryOnlyForLookupFailures) does a good job asserting what must not trigger the fallback (auth errors, unreachable errors, generic errors). The comment above the function is explicit. This is the most security-critical decision in the PR and it's the right call. Worth making sure the relevant RPC layer truly only returns ErrResolveLookup for "no such capability" and not for any auth-adjacent failure, but the test coverage for the negative cases here is exactly what it should look like.
cleanEntityID in servers/sandbox/server.go is a local duplicate. It handles a fixed prefix list (sandbox/, app_version/, app/, pool/) whereas ui.CleanEntityID (used everywhere else) presumably covers the same or broader set. Since the server is in a package that can't import ui without introducing a cycle, this might be necessary — but if the two ever diverge, the inventory will show inconsistently cleaned IDs. A comment linking them would help.
The test coverage for the core security guarantee (canary value must not appear in output) is present at three layers: server_test.go, sandbox_list_test.go, and blackbox/sandbox_test.go. That's the right defence-in-depth for a leak fix.
This is a draft and not yet merge-ready, but the substance of the change is sound. The caveats above are things worth addressing before this leaves draft — they're not blocking the design direction.
panic("not implemented") is generated code and the Export() method that returns this type isn't called anywhere right now, but there's no comment explaining that. Someone unfamiliar with the code-gen pattern will see an exported, callable method with a panic and have no idea if it's a stub they should avoid or a bug. Please add a brief comment explaining that this path is a code-gen artifact and is not intended to be called server-side.cleanEntityID here strips a fixed set of four prefixes (sandbox/, app_version/, app/, pool/) but ui.CleanEntityID (used everywhere else in the codebase) is the canonical implementation. If the two ever diverge — e.g. a new entity kind prefix is added to the UI helper but not here — the inventory server will return un-cleaned IDs while the legacy path returns cleaned ones. Add a comment linking to ui.CleanEntityID and explaining why this copy exists (presumably to avoid a package cycle), and consider whether there's a lower-level helper both can share.sandboxTerminal checks for the normalized strings "stopped" and "dead". This is correct for the RPC path (where the server normalizes via strings.TrimPrefix) and the legacy path (where ui.CleanStatus normalizes). The correctness depends on these two normalization paths being equivalent for all current and future status values. A comment here asserting that assumption — or a unit test that feeds a raw "status.dead" string through listSandboxesFromEntities and checks the filter counts — would make the invariant explicit.Verdict: caveats