The blocking bug I flagged in my last review — the missing SERVICE value in the plain-text route list rows — has been fixed in commit e4e74f0. I can see it directly in the annotated diff: routeService(route) is now inserted at line 124 of cli/commands/route_list.go, between appDisplay and defaultDisplay, exactly matching the position of "SERVICE" in the 9-element header slice. The column count is now consistent.
The author also extended the blackbox test (blackbox/route_service_test.go) to check text output, checking for both the "SERVICE" header and the service value "echo" in the rendered table. That's a meaningful addition — the JSON path was already covered; the text path was the gap.
The docs update is solid too: the --service flag is now documented in route-set.md with a proper Docusaurus :::info admonition for the service constraint, matching the project's conventions.
My earlier secondary observations (the "web" literal default vs. empty-string legacy distinction, and the zero-port HTTPService path) remain accurate but neither is a bug — they're deliberate trade-offs the code comments acknowledge, and I'm not raising them as blocking concerns now.
This is ready to merge.
Posted to the PR's comment threads when you submit.
routeService(route) is now present at line 124 in the plain-text row, in the right position between appDisplay and defaultDisplay. The column count matches the 9-element header. Fixed.Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1095,
"verdict": "ready",
"event": "approve",
"summary": "The blocking bug I flagged in my last review — the missing `SERVICE` value in the plain-text `route list` rows — has been fixed in commit `e4e74f0`. I can see it directly in the annotated diff: `routeService(route)` is now inserted at line 124 of `cli/commands/route_list.go`, between `appDisplay` and `defaultDisplay`, exactly matching the position of `\"SERVICE\"` in the 9-element header slice. The column count is now consistent.\n\nThe author also extended the blackbox test (`blackbox/route_service_test.go`) to check text output, checking for both the `\"SERVICE\"` header and the service value `\"echo\"` in the rendered table. That's a meaningful addition — the JSON path was already covered; the text path was the gap.\n\nThe docs update is solid too: the `--service` flag is now documented in `route-set.md` with a proper Docusaurus `:::info` admonition for the service constraint, matching the project's conventions.\n\nMy earlier secondary observations (the `\"web\"` literal default vs. empty-string legacy distinction, and the zero-port `HTTPService` path) remain accurate but neither is a bug — they're deliberate trade-offs the code comments acknowledge, and I'm not raising them as blocking concerns now.\n\nThis is ready to merge.",
"replies": [
{
"thread_id": "PRRT_kwDONZ__ss6dBwic",
"body": "Confirmed — `routeService(route)` is now present at line 124 in the plain-text row, in the right position between `appDisplay` and `defaultDisplay`. The column count matches the 9-element header. Fixed.",
"resolve": true,
"resolve_reason": "addressed"
}
],
"posted_to_pr": true
}The feature design is solid — the backward-compatibility strategy (empty service field defaults to web everywhere) is thought through, HTTPService validation at the CLI layer is correct, the lease-cache key is properly namespaced per service, and the unit tests for routeService, leaseCacheKey, and HTTPService cover the important cases. One concrete bug needs to be fixed before this ships.
route list table is misalignedcli/commands/route_list.go adds "SERVICE" to the header slice (line 86), making it 9 columns, but the ui.Row built for each route in the plain-text path still only pushes 8 values — it inserts host, appDisplay, defaultDisplay, wafDisplay, timeoutDisplay, servingDisplay, createdAt, updatedAt and skips the service value entirely (lines 119–128). The JSON path is fine. The table render will either produce visually wrong output (column data shifted left under the wrong headers) or panic depending on what ui.AutoSizeColumns / ui.NewTable does with mismatched lengths. The blackbox test only checks JSON output (--format json), so this would slip past CI.
The fix is to insert routeService(route) into the ui.Row after appDisplay, matching the position of the "SERVICE" header.
--service default stores "web" literally. The CLI defines default:"web" on the flag, so a plain route set example.com myapp now writes Service: "web" to the entity store, whereas older routes stored Service: "". Both display as web, routing is identical, but any tool that reads the raw entity will now see a non-empty field where it used to be absent. This is a deliberate trade-off (the comment acknowledges it), but worth being conscious of — if SetRoute ever needs to distinguish "explicitly set to web" from "legacy empty", that information is gone.
HTTPService with len(svc.Ports) == 0 && svc.Port == 0. When a service has neither Port nor Ports, the condition len(svc.Ports) == 0 && svc.Port > 0 is false, and the loop falls through to the else branch which iterates zero ports and never returns nil. The function then returns "has no HTTP port". That seems intentional — a service with no port config at all isn't HTTP-capable — but a future reader might be surprised that a zero-port service hits the "no HTTP port" path rather than a more descriptive error. Not a bug, just a subtle edge.
SERVICE column was added to the headers (line 86) making it 9 columns, but this ui.Row only appends 8 values — the service value is absent. The table will render with misaligned columns. Insert routeService(route) between appDisplay and defaultDisplay to match the header order.Verdict: not_ready