From #8241 (review)
-
internal/pkg/routines/client.go (+390 LOC) — no unit tests
The data-plane client (CRUD, pagination, enable/disable GET+PUT fallback, dispatchAsync, list runs, custom HTTP/2 transport, same-origin pagination guard) has zero tests. PR description defers "unit tests as follow-up", but the manifest parser (low-risk) gets tests while the mission-critical client does not. Suggest at minimum httptest.NewServer-based tests for: 2xx happy paths, 404/409/5xx, nextLink pagination, validateSameOrigin rejection, context cancellation.
-
Command-layer coverage gaps
No tests for routine_update.go (complex flag→file→default merge), routine_delete, routine_enable, routine_disable, routine_dispatch, routine_list, routine_show, routine_helpers. Agents extension test ratio is ~1:1; this PR is ~1:2.2.
-
internal/pkg/routines/client.go — runtime.NewResponseError(resp) leaks raw response body into errors
Used in ~5 spots. Service error bodies pass straight through. CLI commands re-wrap with exterrors.ServiceFromAzure, but the underlying message text is preserved and may surface in logs/output.
Fix: drain body, extract structured fields, return a controlled message — or document that callers must always wrap and never log the raw error.
🟢 Low / nit
9. internal/cmd/endpoint_test.go — port edge cases under-tested
The recently-added port rejection covers :443 only. Add: non-default ports (:8443), IPv6+port (https://[::1]:443/...), whitespace-only input. The original bug evaded tests because they didn't exercise non-default ports.
-
internal/cmd/routine_update.go — _ = cmd.Flags().MarkHidden(...) (×2)
Silent error discard. Safe in practice, but if err := ...; err != nil { ... } or a brief comment is cleaner.
-
AGENTS.md — "If this extension grows enough… introduce internal/exterrors"
Phrased as a future option, but the package exists in this PR. Rewrite in present tense so future maintainers don't treat it as optional.
-
go.mod — armappservice/v2 v2.3.0 // indirect added
Indirect, likely a cascade from an azd-core dep bump. Worth a quick confirmation that this isn't pulling unwanted weight on the extension.
From #8241 (review)
internal/pkg/routines/client.go (+390 LOC) — no unit tests
The data-plane client (CRUD, pagination, enable/disable GET+PUT fallback, dispatchAsync, list runs, custom HTTP/2 transport, same-origin pagination guard) has zero tests. PR description defers "unit tests as follow-up", but the manifest parser (low-risk) gets tests while the mission-critical client does not. Suggest at minimum httptest.NewServer-based tests for: 2xx happy paths, 404/409/5xx, nextLink pagination, validateSameOrigin rejection, context cancellation.
Command-layer coverage gaps
No tests for routine_update.go (complex flag→file→default merge), routine_delete, routine_enable, routine_disable, routine_dispatch, routine_list, routine_show, routine_helpers. Agents extension test ratio is ~1:1; this PR is ~1:2.2.
internal/pkg/routines/client.go — runtime.NewResponseError(resp) leaks raw response body into errors
Used in ~5 spots. Service error bodies pass straight through. CLI commands re-wrap with exterrors.ServiceFromAzure, but the underlying message text is preserved and may surface in logs/output.
Fix: drain body, extract structured fields, return a controlled message — or document that callers must always wrap and never log the raw error.
🟢 Low / nit
9. internal/cmd/endpoint_test.go — port edge cases under-tested
The recently-added port rejection covers :443 only. Add: non-default ports (:8443), IPv6+port (https://[::1]:443/...), whitespace-only input. The original bug evaded tests because they didn't exercise non-default ports.
internal/cmd/routine_update.go — _ = cmd.Flags().MarkHidden(...) (×2)
Silent error discard. Safe in practice, but if err := ...; err != nil { ... } or a brief comment is cleaner.
AGENTS.md — "If this extension grows enough… introduce internal/exterrors"
Phrased as a future option, but the package exists in this PR. Rewrite in present tense so future maintainers don't treat it as optional.
go.mod — armappservice/v2 v2.3.0 // indirect added
Indirect, likely a cascade from an azd-core dep bump. Worth a quick confirmation that this isn't pulling unwanted weight on the extension.