Partial revert coderabbit added docstrings

client/mdm/policy.go

@@ -82,12 +82,9 @@ type Policy struct {
 	values map[string]any
 }
 
-// NewPolicy constructs a Policy from a key→value map. Pass nil or an empty
-// NewPolicy constructs a Policy backed by the provided key→value map.
-// If values is nil it is replaced with an empty map so the returned *Policy
-// NewPolicy constructs a non-nil *Policy that wraps the provided key/value map.
-// If values is nil it is replaced with an empty map so the returned Policy always
-// represents "no MDM enforcement" when its values are empty.
+// NewPolicy constructs a Policy from a key→value map. Pass nil or an
+// empty map to construct an empty (no-enforcement) Policy. The returned
+// *Policy is always non-nil.
 func NewPolicy(values map[string]any) *Policy {
 	if values == nil {
 		values = map[string]any{}
@@ -95,20 +92,14 @@ func NewPolicy(values map[string]any) *Policy {
 	return &Policy{values: values}
 }
 
-// LoadPolicy reads the platform-native MDM configuration. Returns an empty
-// (but non-nil) Policy when no source is present, the source is empty, or
-// the platform is unsupported.
+// LoadPolicy reads the platform-native MDM configuration. Returns an
+// empty (but non-nil) Policy when no source is present, the source is
+// empty, or the platform is unsupported.
 //
 // Diagnostic logging differentiates the three states:
 //   - source absent / unsupported platform: trace log only
 //   - source present, zero keys:             info "MDM enrolled (no managed keys)"
-// LoadPolicy loads MDM-managed configuration from the platform and returns a Policy representing the managed settings.
-// If the platform loader fails or returns nil, LoadPolicy returns a non-nil empty Policy.
-// LoadPolicy loads platform-managed MDM key/value pairs and returns a non-nil Policy.
-// If the platform loader returns an error or a nil map, an empty Policy is returned.
-// On loader error a trace-level message is emitted. When a map is returned, an
-// informational message is logged either indicating enrollment with no managed keys
-// or the count and a stable, sorted list of managed key names.
+//   - source present, N keys:                info "MDM enrolled with N managed keys: [...]"
 func LoadPolicy() *Policy {
 	values, err := loadPlatformPolicy()
 	if err != nil {
@@ -266,8 +257,7 @@ func (p *Policy) GetStringSlice(key string) ([]string, bool) {
 // sortedKeys returns the keys of m as a deterministic, lexicographically
 // sorted slice. Used internally by Policy.ManagedKeys and LoadPolicy's
 // diagnostic log line so callers see a stable key order across runs
-// sortedKeys returns the keys of m as a lexicographically sorted slice.
-// The sorted order provides a deterministic key ordering for diagnostics and enumeration.
+// regardless of Go's randomised map iteration.
 func sortedKeys(m map[string]any) []string {
 	out := make([]string, 0, len(m))
 	for k := range m {

client/mdm/policy_darwin.go

@@ -26,28 +26,19 @@ import (
 const policyPlistPath = "/Library/Managed Preferences/io.netbird.client.plist"
 
 // loadPlatformPolicy reads the MDM-managed configuration from the macOS
-// managed-preferences plist. Returns:
+// managed-preferences plist at policyPlistPath. Returns:
 //   - (nil, nil)  when the plist is absent (device not MDM-enrolled for
 //     NetBird, or admin has not yet pushed a payload)
 //   - (map, nil)  with N entries when N managed values are present
 //     (N may be 0 — empty plist still signals enrollment to the caller)
-//   - (nil, err)  on permission / parse / safety errors
+//   - (nil, err)  on permission / parse / safety errors (including
+//     refusal to read a world-writable plist)
 //
-// Value-type coercion mirrors the Windows loader: native plist types
-// map naturally onto the Policy accessor expectations (GetString /
-// GetBool / GetInt / GetStringSlice). Unknown top-level keys are
-// logged and skipped so a stray entry in the payload does not block
-// loadPlatformPolicy reads the managed-preferences plist at policyPlistPath and returns recognised MDM key/value pairs.
-//
-// If the plist file does not exist, it returns (nil, nil). It returns a wrapped error on open/stat/decode failures.
-// The function refuses to read a world-writable plist and returns an error in that case.
-// loadPlatformPolicy reads the device-level managed-preferences plist and returns its recognized keys.
-// 
-// It looks for the plist at policyPlistPath and, if present, decodes it into a map[string]any.
-// Top-level plist keys are canonicalized case-insensitively to the package's internal MDM key names;
-// unknown keys are logged and ignored. If the plist file does not exist, it returns (nil, nil).
-// The function refuses to read the file if it is world-writable and returns a wrapped error for
-// failures to open, stat, or decode the plist.
+// Top-level plist keys are canonicalised case-insensitively to the
+// package's internal mdm.Key* names; unknown keys are logged and
+// skipped so a stray entry in the payload does not block startup.
+// Native plist value types map naturally onto the Policy accessor
+// expectations (GetString / GetBool / GetInt / GetStringSlice).
 func loadPlatformPolicy() (map[string]any, error) {
 	f, err := os.Open(policyPlistPath)
 	if err != nil {

client/mdm/policy_windows.go

@@ -17,27 +17,17 @@ import (
 // Listed in the project's docs/mdm/netbird.admx schema.
 const policyRegistryPath = `Software\Policies\NetBird`
 
-// loadPlatformPolicy reads the MDM-managed configuration from the Windows
-// registry under HKLM\Software\Policies\NetBird. Returns:
-//   - (nil, nil)  when the key is absent (device not MDM-enrolled for NetBird)
-//   - (map, nil)  with N entries when N managed values are set (N may be 0)
-//   - (nil, err)  on any other registry error
+// readRegistryValue reads a single value under policyRegistryPath and,
+// on success, stores the type-coerced result in out[canonical]. Type
+// coercion mirrors loadPlatformPolicy's documented mapping:
+//   - REG_SZ / REG_EXPAND_SZ -> string (REG_EXPAND_SZ is expanded by the API)
+//   - REG_DWORD / REG_QWORD  -> int64
+//   - REG_MULTI_SZ           -> []string
 //
-// Type coercion of registry value types into the Policy map:
-//   - REG_SZ        -> string
-//   - REG_EXPAND_SZ -> string (expanded by the registry API)
-//   - REG_DWORD     -> int64 (caller's GetBool handles 0/!=0 coercion)
-//   - REG_QWORD     -> int64
-//   - REG_MULTI_SZ  -> []string
-//
-// Unsupported value types (REG_BINARY, REG_NONE, ...) are skipped with a
-// loadPlatformPolicy reads managed NetBird policy values from HKLM\Software\Policies\NetBird.
-// If the registry key does not exist it returns (nil, nil).
-// It returns a map whose keys are canonical policy names and whose values are coerced from registry types:
-// REG_SZ/REG_EXPAND_SZ -> string, REG_DWORD/REG_QWORD -> int64, REG_MULTI_SZ -> []string.
-// readRegistryValue reads the registry value named by name from key k and, when the value is successfully read and its type is supported, stores the coerced Go value in out[canonical].
-//
-// REG_SZ and REG_EXPAND_SZ are stored as string, REG_DWORD and REG_QWORD are stored as int64, and REG_MULTI_SZ is stored as []string; unknown value names, unsupported value types, and per-value read errors are logged and skipped.
+// Unsupported value types and per-value read failures are logged at
+// warn level and skipped — one malformed value must not block the
+// surrounding loop. Extracted from loadPlatformPolicy to keep that
+// function's cognitive complexity in check.
 func readRegistryValue(k registry.Key, name, canonical string, out map[string]any) {
 	_, valType, err := k.GetValue(name, nil)
 	if err != nil {
@@ -71,16 +61,15 @@ func readRegistryValue(k registry.Key, name, canonical string, out map[string]an
 	}
 }
 
-// loadPlatformPolicy loads MDM-managed NetBird policy values from the Windows
-// registry at HKLM\Software\Policies\NetBird.
-// 
-// It returns a map that maps canonical policy names to coerced Go values:
-// string for REG_SZ/REG_EXPAND_SZ, int64 for REG_DWORD/REG_QWORD, and []string
-// for REG_MULTI_SZ. If the policy registry key does not exist, it returns
-// (nil, nil). It returns an error when opening or enumerating the registry
-// key fails. Individual values that are unknown, of unsupported types, or that
-// fail to read are skipped and produce logged warnings; registry close failures
-// are also logged.
+// loadPlatformPolicy reads the MDM-managed configuration from the
+// Windows registry under HKLM\Software\Policies\NetBird. Returns:
+//   - (nil, nil)  when the key is absent (device not MDM-enrolled for NetBird)
+//   - (map, nil)  with N entries when N managed values are set (N may be 0)
+//   - (nil, err)  on open / enumerate registry errors
+//
+// Per-value type coercion + skip-on-error is delegated to
+// readRegistryValue. Unknown value names are logged and skipped so a
+// malformed deployment does not block startup.
 func loadPlatformPolicy() (map[string]any, error) {
 	k, err := registry.OpenKey(registry.LOCAL_MACHINE, policyRegistryPath, registry.QUERY_VALUE)
 	if err != nil {

client/mdm/ticker.go

@@ -24,10 +24,9 @@ const defaultReloadInterval = 1 * time.Minute
 const testReloadInterval = 1 * time.Second
 
 // reloadInterval returns the production cadence, or the accelerated test
-// cadence when running under `go test`. Centralising the choice here keeps
-// reloadInterval selects the polling interval used to re-read the OS-native MDM policy.
-// reloadInterval selects the polling interval used for policy reloads.
-// It returns testReloadInterval when running under `go test` (testing.Testing() == true), and defaultReloadInterval otherwise.
+// cadence when running under `go test` (detected via testing.Testing()).
+// Centralising the choice here keeps the prod/test split in one place
+// and out of the ticker's call sites.
 func reloadInterval() time.Duration {
 	if testing.Testing() {
 		return testReloadInterval
@@ -51,16 +50,15 @@ type Ticker struct {
 	prev     *Policy
 }
 
-// NewTicker constructs a Ticker that re-reads the OS-native policy every
-// reloadInterval() and invokes onChange on any diff. The cadence is owned by
-// reloadInterval (production default, accelerated under `go test`); callers
-// NewTicker creates a Ticker that polls the OS-native MDM policy at the package reload interval and invokes onChange when a policy change is detected.
-// If onChange is nil the ticker will only log detected changes.
-// NewTicker creates a Ticker that polls for policy changes and invokes onChange when a difference is detected.
-// 
-// The provided onChange callback, if non-nil, is called with the previous and current Policy snapshots when a
-// change is observed. The returned Ticker's polling interval is set via reloadInterval and its initial snapshot
-// is populated by calling policyLoader.
+// NewTicker constructs a Ticker that re-reads the OS-native policy
+// every reloadInterval() and invokes onChange on any diff. The
+// cadence is owned by reloadInterval (production default, accelerated
+// under `go test`); callers do not supply it. onChange may be nil for
+// a log-only ticker. The initial snapshot is populated by calling
+// policyLoader at construction time so the first tick only fires
+// onChange when the policy actually changed since boot — without
+// this baseline the first tick would report every currently-managed
+// key as "added" and trigger a spurious engine restart.
 func NewTicker(onChange func(prev, curr *Policy)) *Ticker {
 	return &Ticker{
 		interval: reloadInterval(),
@@ -98,11 +96,10 @@ func (t *Ticker) Run(ctx context.Context) {
 	}
 }
 
-// PoliciesEqual reports whether two Policy instances carry the same managed
-// PoliciesEqual reports whether two Policy instances represent the same policy.
-// It returns true when both policies are empty, returns false if one pointer is nil
-// while the other is not, and otherwise compares the policies' underlying value
-// maps for deep equality.
+// PoliciesEqual reports whether two Policy instances carry the same
+// managed key set with identical values. Nil and empty policies
+// compare equal; one-nil/one-non-empty compare not equal; otherwise
+// the underlying values maps are compared with reflect.DeepEqual.
 func PoliciesEqual(a, b *Policy) bool {
 	if a.IsEmpty() && b.IsEmpty() {
 		return true
@@ -113,12 +110,10 @@ func PoliciesEqual(a, b *Policy) bool {
 	return reflect.DeepEqual(a.values, b.values)
 }
 
-// diffPolicies returns the keys added in curr, removed from prev, and whose
-// diffPolicies reports keys that were added, removed, or changed between two policies.
-// The returned slices contain keys present only in `curr` (added), only in `prev` (removed),
-// and present in both but whose values differ (changed). Each slice is sorted
-// lexicographically for stable logging output; value differences are determined
-// associated values differ by deep equality.
+// diffPolicies returns the keys added in curr, removed from prev, and
+// whose values changed between prev and curr. Each slice is sorted
+// lexicographically for stable log output; value differences are
+// determined with reflect.DeepEqual.
 func diffPolicies(prev, curr *Policy) (added, removed, changed []string) {
 	prevKeys := mapOf(prev)
 	currKeys := mapOf(curr)
@@ -140,13 +135,9 @@ func diffPolicies(prev, curr *Policy) (added, removed, changed []string) {
 	return added, removed, changed
 }
 
-// mapOf returns a (possibly empty, never nil) copy of the underlying values
-// map of a Policy so callers outside this package can compare across the
-// mapOf returns a non-nil copy of the given Policy's key/value map.
-// If p is nil, mapOf returns an empty map; otherwise it returns a newly
-// mapOf returns a non-nil copy of a Policy's values map.
-// If p is nil it returns an empty map; otherwise it returns a newly
-// allocated map containing the same key/value pairs as p.values.
+// mapOf returns a (possibly empty, never nil) copy of the underlying
+// values map of a Policy so callers outside this package can compare
+// keys/values across the type boundary. Returns an empty map on nil p.
 func mapOf(p *Policy) map[string]any {
 	if p == nil {
 		return map[string]any{}