audit: config library — violations, issues, smells #12

New issue

Open

opened 2026-09-21 19:19:03 +00:00 by agent · 0 comments

agent commented

2026-09-21 19:19:03 +00:00

Member

audit — config library

summary

The config library is well-structured with a clean API surface and strong test coverage (93 tests). However, there are three categories of concern: (1) a broken wildcard matching system that tests pass by coincidence, (2) systematic silent error swallowing that makes config debugging nearly impossible, and (3) an inconsistency between the simple and rules-based merge paths that causes conf:"-" fields to be merged when they shouldn't be. The homeDir() fallback to "/" is a silent data-corruption risk.

violations

[VIOLATION] core/merge.go:118-157 — scope discipline — mergeStructs iterates by raw index, merging ALL fields including conf:"-" excluded fields. mergeStructsWithRules correctly skips them via f.Skip || f.RawExclude. Users of UseConfig/Overwrite get different behavior than MergeWithRules for the same struct. — Make mergeStructs use inspectFields to skip excluded fields, or route all merges through mergeStructsWithRules with nil rules.
[VIOLATION] core/merge.go:279-288 — more than nothing — Wildcard patterns like database.* can never match because inspectFields returns flat leaf names (e.g., "host"), not dotted paths (e.g., "database.host"). The TestMergeWithRulesWildcard test passes by coincidence: cascade is the default, so the rule not matching produces the same result as it matching. — Either make inspectFields return dotted paths for nested fields, or change the wildcard docs to reflect that only flat names are matched.
[VIOLATION] core/app.go:102-110 — more than nothing — homeDir() falls back to "/" when both os.UserHomeDir() and $HOME fail, causing defaultUserDir() to return "/.config". Any write operation would attempt to create files at the filesystem root. This is a silent data-corruption risk. — Return an error from defaultUserDir(), or panic, or at minimum log a warning.

issues

[ISSUE] core/env.go:84-87 — more than nothing — coerceAndSet silently swallows all parse errors. A user setting APP__PORT=abc gets no error and no warning — the old value silently persists. This pattern repeats across the library (F8, F13, F16, F17, F24). — Return errors from coerceAndSet and propagate them. At minimum, log a warning.
[ISSUE] core/load.go:105-106 — more than nothing — InitWithTemplate discards errors from SystemRaw and UserRaw with _. A malformed system config file is silently ignored, and the cascade proceeds with zero values. Compare with Custom[T] which does return errors. — Surface these errors, or at minimum log them.
[ISSUE] core/load.go:201-229 — touch only what is necessary — loadRaw is dead code in production. It is unexported and called only from one test (load_test.go:22). The doc comment says "planned entry point for MergeWithRules" — a feature that was completed without it. — Remove loadRaw or fold its unique behavior into loadFromPathRaw.
[ISSUE] core/tag.go:44,232 — touch only what is necessary — fieldMeta.PkgPath is set at line 232 but never read by any code. The PkgPath() checks in buildShadowType operate on reflect.Type, not fieldMeta. — Remove the field.
[ISSUE] core/template.go:14 — more than nothing — The placeholderRe regex requires uppercase placeholders ([A-Z0-9_]). ${CONF__console__shell} does NOT match. This is not documented in the RenderTemplate doc comment or README. — Document the uppercase requirement, or make the regex case-insensitive.
[ISSUE] stubs.go:56,62 — no ceremony — UseConfig and CascadeEnv are byte-for-byte identical functions. The doc comment attempts a semantic distinction but the implementation is the same. This is API surface bloat. — Remove CascadeEnv or make it a type alias.

smells

[SMELL] core/tag.go:52-59 — inspectFields is not cached. It is called 6-8 times per type during a single InitWithTemplate call. The shadowCache caches shadow types but not field metadata. For deeply nested structs this compounds. — Cache []fieldMeta by reflect.Type.
[SMELL] core/env.go:172-173 — splitEnvSep calls strings.TrimRight(s, "\r\n") then strings.TrimSpace(s). The first call is redundant since TrimSpace already trims \r and \n. — Remove the TrimRight call.
[SMELL] core/write.go:177-183 — isRoot() uses user.Current() which does NSS lookups. os.Geteuid() == 0 is simpler and faster. — Replace with os.Geteuid() == 0.
[SMELL] core/app.go:88-90 — defaultSystemDir() wraps the constant "/etc" in a function. Adds indirection for no value. — Make it a const.
[SMELL] core/load.go:231-242 — ResidualConfig is defined in load.go but used across merge.go, env.go, write.go, and stubs.go. It would be more natural in its own file. — Move to types.go or similar.
[SMELL] core/env.go:189 — implementsTextUnmarshaler allocates a reflect.Type on every call. Not cached. Minor in config loading but easy to fix. — Cache the result.
[SMELL] core/codec.go:45-48 — TOML decoder discards MetaData which contains undecoded keys. A config file with unrecognized keys silently ignores them. — Consider logging or returning undecoded keys.

notes

[NOTE] core/env.go:119-163 — setSlice and coerceAndSet do not handle Uint/Uint64 types. Undocumented limitation.
[NOTE] core/tag.go:108 — omitempty accepts "1" and "yes" in addition to "true". Undocumented but more permissive is generally fine.
[NOTE] core/merge.go:50-53 — The wildcard limitation (leaf-only matching) is documented in code comments but not in README. The README says "section.* matches all fields under section" which is misleading.
[NOTE] No race conditions detected. shadowCache uses sync.Map. All other state is function-local.
[NOTE] No resource leaks detected. All file operations use os.ReadFile/os.WriteFile which handle cleanup.
[NOTE] core/tag.go:193-199 — Panic on malformed conf tags is unreachable via normal Go struct tags (Go's reflect.StructTag.Lookup truncates at the first "). The panic is defense-in-depth but would crash the program if it ever fired.

verification

build: PASS — go build ./...
tests: PASS — go test -count=1 ./... (93 tests)
vet: PASS — go vet ./...

## audit — config library ### summary The config library is well-structured with a clean API surface and strong test coverage (93 tests). However, there are three categories of concern: (1) a broken wildcard matching system that tests pass by coincidence, (2) systematic silent error swallowing that makes config debugging nearly impossible, and (3) an inconsistency between the simple and rules-based merge paths that causes `conf:"-"` fields to be merged when they shouldn't be. The `homeDir()` fallback to `"/"` is a silent data-corruption risk. ### violations - [VIOLATION] `core/merge.go:118-157` — scope discipline — `mergeStructs` iterates by raw index, merging ALL fields including `conf:"-"` excluded fields. `mergeStructsWithRules` correctly skips them via `f.Skip || f.RawExclude`. Users of `UseConfig`/`Overwrite` get different behavior than `MergeWithRules` for the same struct. — Make `mergeStructs` use `inspectFields` to skip excluded fields, or route all merges through `mergeStructsWithRules` with nil rules. - [VIOLATION] `core/merge.go:279-288` — more than nothing — Wildcard patterns like `database.*` can never match because `inspectFields` returns flat leaf names (e.g., `"host"`), not dotted paths (e.g., `"database.host"`). The `TestMergeWithRulesWildcard` test passes by coincidence: cascade is the default, so the rule not matching produces the same result as it matching. — Either make `inspectFields` return dotted paths for nested fields, or change the wildcard docs to reflect that only flat names are matched. - [VIOLATION] `core/app.go:102-110` — more than nothing — `homeDir()` falls back to `"/"` when both `os.UserHomeDir()` and `$HOME` fail, causing `defaultUserDir()` to return `"/.config"`. Any write operation would attempt to create files at the filesystem root. This is a silent data-corruption risk. — Return an error from `defaultUserDir()`, or panic, or at minimum log a warning. ### issues - [ISSUE] `core/env.go:84-87` — more than nothing — `coerceAndSet` silently swallows all parse errors. A user setting `APP__PORT=abc` gets no error and no warning — the old value silently persists. This pattern repeats across the library (F8, F13, F16, F17, F24). — Return errors from `coerceAndSet` and propagate them. At minimum, log a warning. - [ISSUE] `core/load.go:105-106` — more than nothing — `InitWithTemplate` discards errors from `SystemRaw` and `UserRaw` with `_`. A malformed system config file is silently ignored, and the cascade proceeds with zero values. Compare with `Custom[T]` which does return errors. — Surface these errors, or at minimum log them. - [ISSUE] `core/load.go:201-229` — touch only what is necessary — `loadRaw` is dead code in production. It is unexported and called only from one test (`load_test.go:22`). The doc comment says "planned entry point for MergeWithRules" — a feature that was completed without it. — Remove `loadRaw` or fold its unique behavior into `loadFromPathRaw`. - [ISSUE] `core/tag.go:44,232` — touch only what is necessary — `fieldMeta.PkgPath` is set at line 232 but never read by any code. The `PkgPath()` checks in `buildShadowType` operate on `reflect.Type`, not `fieldMeta`. — Remove the field. - [ISSUE] `core/template.go:14` — more than nothing — The `placeholderRe` regex requires uppercase placeholders (`[A-Z0-9_]`). `${CONF__console__shell}` does NOT match. This is not documented in the `RenderTemplate` doc comment or README. — Document the uppercase requirement, or make the regex case-insensitive. - [ISSUE] `stubs.go:56,62` — no ceremony — `UseConfig` and `CascadeEnv` are byte-for-byte identical functions. The doc comment attempts a semantic distinction but the implementation is the same. This is API surface bloat. — Remove `CascadeEnv` or make it a type alias. ### smells - [SMELL] `core/tag.go:52-59` — `inspectFields` is not cached. It is called 6-8 times per type during a single `InitWithTemplate` call. The `shadowCache` caches shadow types but not field metadata. For deeply nested structs this compounds. — Cache `[]fieldMeta` by `reflect.Type`. - [SMELL] `core/env.go:172-173` — `splitEnvSep` calls `strings.TrimRight(s, "\r\n")` then `strings.TrimSpace(s)`. The first call is redundant since `TrimSpace` already trims `\r` and `\n`. — Remove the `TrimRight` call. - [SMELL] `core/write.go:177-183` — `isRoot()` uses `user.Current()` which does NSS lookups. `os.Geteuid() == 0` is simpler and faster. — Replace with `os.Geteuid() == 0`. - [SMELL] `core/app.go:88-90` — `defaultSystemDir()` wraps the constant `"/etc"` in a function. Adds indirection for no value. — Make it a `const`. - [SMELL] `core/load.go:231-242` — `ResidualConfig` is defined in `load.go` but used across `merge.go`, `env.go`, `write.go`, and `stubs.go`. It would be more natural in its own file. — Move to `types.go` or similar. - [SMELL] `core/env.go:189` — `implementsTextUnmarshaler` allocates a `reflect.Type` on every call. Not cached. Minor in config loading but easy to fix. — Cache the result. - [SMELL] `core/codec.go:45-48` — TOML decoder discards `MetaData` which contains undecoded keys. A config file with unrecognized keys silently ignores them. — Consider logging or returning undecoded keys. ### notes - [NOTE] `core/env.go:119-163` — `setSlice` and `coerceAndSet` do not handle `Uint`/`Uint64` types. Undocumented limitation. - [NOTE] `core/tag.go:108` — `omitempty` accepts `"1"` and `"yes"` in addition to `"true"`. Undocumented but more permissive is generally fine. - [NOTE] `core/merge.go:50-53` — The wildcard limitation (leaf-only matching) is documented in code comments but not in README. The README says "section.* matches all fields under section" which is misleading. - [NOTE] No race conditions detected. `shadowCache` uses `sync.Map`. All other state is function-local. - [NOTE] No resource leaks detected. All file operations use `os.ReadFile`/`os.WriteFile` which handle cleanup. - [NOTE] `core/tag.go:193-199` — Panic on malformed conf tags is unreachable via normal Go struct tags (Go's `reflect.StructTag.Lookup` truncates at the first `"`). The panic is defense-in-depth but would crash the program if it ever fired. ### verification - build: PASS — `go build ./...` - tests: PASS — `go test -count=1 ./...` (93 tests) - vet: PASS — `go vet ./...`

agent added the

proj-config-lib

label

2026-09-21 19:19:30 +00:00