Skip to content

idiomatic code audit: findings for azd-rest #102

@jongio

Description

@jongio

idiomatic code audit

  • maxPages global variable (root.go:39) is declared and mapped into Config.MaxPages via snapshotConfig() (line 181), but the corresponding --max-pages cobra persistent flag is never registered in NewRootCmd(). This means pagination page limits always default to 0 at CLI invocation, overriding config.Defaults() which sets it to 100. When --paginate is used with --verbose, this produces a misleading "max 0 pages" warning and may silently break pagination behavior depending on how the httpclient handles a zero MaxPages.
  • buildRequestOptions() in root.go (line 211) explicitly discards the file-handle cleanup function with _ = cleanup, with a comment citing backward compatibility for tests that call buildRequestOptions directly. This creates a latent resource leak: callers that open a --data-file body but don't go through executeRequest will leave the file handle open until GC. The cleanup contract should be documented or enforced at the type level (e.g., returning a dedicated type with Close()).
  • resetSecurityPolicyForTest() and setSecurityPolicyForTest() in mcp.go (lines 124-137) reset the securityPolicyOnce sync.Once singleton by direct reassignment (securityPolicyOnce = sync.Once{}). While this is technically safe (replacing the variable, not copying after first use), it is a fragile test-only pattern that signals the security policy initialization should use an injectable factory or interface instead of a package-level sync.Once singleton, which would be the idiomatic Go approach for testable singletons.
  • getVersion() in magefile.go (lines 541-558) parses the extension.yaml file using strings.Split(line, ":") rather than a YAML parser. If the version value ever contains a colon (e.g., an ISO timestamp or a URL-style value), the split produces more than two parts and the function silently returns an error. The project already has gopkg.in/yaml.v3 in go.mod; using it here would be more robust and idiomatic.
  • go.uber.org/atomic v1.11.0 and go.uber.org/multierr v1.11.0 appear as indirect dependencies in go.mod. With go 1.26.1 as the minimum version, both are superseded by standard library features introduced in earlier versions: typed atomic operations (sync/atomic generics since Go 1.19) and errors.Join (since Go 1.20). These originate from upstream transitive dependencies; tracking them for removal once upstream packages update would reduce dependency surface.

Automated analysis - 5 finding(s)

Metadata

Metadata

Assignees

No one assigned

    Labels

    automatedFiled by automated analysis

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions