From 5098b56655bd383e22a57d6cf09d99db3e92090f Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 25 Oct 2026 15:52:43 +0000 Subject: [PATCH] Record functional correctness audit findings --- docs/roadmap/correctness.md | 191 ++++++++++++++++++++++++++++++++++++ 1 file changed, 191 insertions(+) create mode 100644 docs/roadmap/correctness.md diff --git a/docs/roadmap/correctness.md b/docs/roadmap/correctness.md new file mode 100644 index 0000000..4508be3 --- /dev/null +++ b/docs/roadmap/correctness.md @@ -0,0 +1,191 @@ +# Functional Correctness Audit + +Reviewed revision: `47944d22d1ad45a7f537780effb3d05f998eef14` +(Update ReleaseKit to v0.4.2). +Audit date: 2026-10-25. + +## Scope And Evidence + +This audit reviewed the root facade and its engine handoffs, source/discovery +and filesystem handling, composition and collection rules, inheritance, typed +decoding and ownership, application callbacks, secrets, metadata, explanation, +and effective-value comparison against the configuration contract and relevant +implementation/tests. It was a source-level functional review with targeted +diagnostics, not an exhaustive proof or a performance/security certification. + +The existing development check (`./scripts/check.sh`) and full race-enabled +suite (`go test -race ./...`) passed using Go 1.26.10 on Linux amd64. Standalone +public-API probes outside the repository reproduced the findings below. No +production code or repository tests were added or changed. No native macOS or +Windows validation was performed, and no new release gates are proposed. + +## 1. Pointer-Only Recursive Types Can Overflow The Stack Or Hang + +Status: open. Priority: medium. Category: schema/decoder termination. + +**Relevant code.** `schemaBuilder.describe` in +[schema.go](../../internal/engine/schema.go), lines 119–140, caches a placeholder +before describing pointer elements. A legal Go type such as `type Loop *Loop` +therefore produces a pointer descriptor whose element is itself. +`decodeValue` in [decode.go](../../internal/engine/decode.go), lines 93–106, +recursively unwraps pointers without consuming the input node. The pointer +unwrapping loops in `resolveSchemaPath` (`schema.go`, lines 347–350 and 387–390) +also assume that a non-pointer descriptor will eventually be reached. + +**Reproduction.** With the public package imported as `configkit`: + +```go +type Loop *Loop +type Config struct { + Value Loop `yaml:"value"` +} + +configkit.Load[Config](configkit.Bytes("input", []byte("value: {}\n"))) +``` + +This does not return a decode error. It repeatedly calls `decodeValue` with the +same input and descriptor until the process reports a fatal stack overflow. +A disposable probe using `debug.SetMaxStack(64 << 10)` demonstrated the failure +without allowing the process to grow a large stack. Explicit `value: null` +loads successfully. With that null input, adding a resolver that calls +`Normalize` on `/value` with `Loop(nil)` instead hangs in `resolveSchemaPath`; +the separate diagnostic process was stopped by a three-second timeout. + +**Expected behavior.** Incompatible non-null input must fail with a safe +diagnostic, and schema-path resolution must terminate. The +[schema contract](../config.md#application-schema-binding) supports recursive +types with acyclic values; the existing explicit pointer-only restriction is +for inline fields, not all pointer fields. This is not a request to recover +application panics or reject ordinary recursive structs. + +**Potential remediation.** Detect repeated descriptors while unwrapping +pointer-only chains in decoding and schema-path traversal. Preserve supported +nil/null values and finite acyclic values, return an appropriate decode error +for an incompatible non-null node, and reject descendants that cannot be +addressed. Review analogous pointer-unwrapping helpers for the same assumption. +A blanket schema rejection of pointer-only recursive types would instead be a +contract decision, since it would also reject currently valid nil values; do +not introduce it or an arbitrary pointer-depth cap merely to stop the loop. + +## 2. Failed Dynamic Schema Construction Leaves A Reusable Partial Descriptor + +Status: open. Priority: medium. Category: error handling and shared operation state. + +**Relevant code.** `valueOperation.dynamicDescriptor` in +[value_operation.go](../../internal/engine/value_operation.go), lines 36–48, +passes the load's existing descriptor cache directly to a new schema builder. +`schemaBuilder.describe` in [schema.go](../../internal/engine/schema.go), lines +119–140 and 167–191, caches container placeholders but returns child errors +without removing those placeholders. A subsequent lookup returns the partial +descriptor as if construction had succeeded. + +**Reproduction.** Use a configuration with `Value any` and this dynamic type: + +```go +type Invalid struct { + Channel chan int `yaml:"channel"` +} +``` + +In one resolver, execute these two assignments in order: + +```go +configkit.Normalize(r, configkit.Keys("value"), &c.Value, + any((*Invalid)(nil)), "first unsupported value") +configkit.Normalize(r, configkit.Keys("value"), &c.Value, + any(&Invalid{}), "second unsupported value") +``` + +The first helper correctly records an invalid-value diagnostic because channels +are unsupported. It nevertheless leaves the `*Invalid` descriptor cached with +a nil element. The second helper reuses it, reaches projection with that nil +element, and panics with a nil-pointer dereference instead of returning a +`LoadError`. This was reproduced through ordinary public `Load` and `Normalize`; +the application callback itself does not panic. + +**Expected behavior.** Unsupported dynamic/helper values must produce safe +schema/resolution diagnostics under the +[typed projection](../config.md#typed-projection-and-ownership) and +[resolution contracts](../config.md#recorded-application-resolution). +One reported validation failure must not corrupt the descriptor cache used by +later operations in the same callback. + +**Potential remediation.** Make dynamic descriptor construction transactional: +stage newly discovered descriptors and commit them only after a successful +build, or track and roll back all cache additions belonging to a failed build. +Account for recursive descriptors that refer to other partially built types, +not just the immediate failed container. Preserve previously valid descriptors +and the load-local cache ownership. A nil guard in projection alone would not +repair invalid cache entries being treated as successful schema descriptions. + +## 3. Cycle Detection Rejects Acyclic Slice Views Sharing A Starting Address + +Status: open. Priority: medium. Category: supported-value correctness. + +**Relevant code.** `valueVisit` and `visitFor` in +[value_operation.go](../../internal/engine/value_operation.go), lines 20–24 +and 97–116, identify a slice by its type, kind, and starting pointer, but not its +length. Both reflective projection/cloning and the parser-neutral value builder +in [value.go](../../internal/engine/value.go) use that identity for active-path +cycle detection. + +**Reproduction.** This value is finite and acyclic: its nested one-element view +contains only the string, not the enclosing two-element view. + +```go +shared := []any{"leaf", nil} +shared[1] = shared[:1] +``` + +Place it at `data` in a `Mapping` source, return it from typed defaults for +`Data []any`, or supply it as a CLI override. Observed results at `/data/1` were, +respectively, `source.invalid`, `schema.cycle`, and `override.invalid_value`. +The equivalent independently allocated value +`[]any{"leaf", []any{"leaf"}}` loads and renders successfully. A true +self-referential slice was also checked and remains correctly rejected. + +**Expected behavior.** The +[mapping source](../config.md#sources), +[projection/ownership](../config.md#typed-projection-and-ownership), and +[override](../config.md#explicit-cli-overrides) contracts permit supported +acyclic values. Shared backing storage alone is not a cycle, and detaching it +does not require preserving alias identity in the returned configuration. + +**Potential remediation.** Include the visible slice extent (its length) in the +active-visit identity, or use an equivalent identity that distinguishes these +views. Preserve detection of actual self/mutual cycles, empty/nil distinctions, +and independent ownership of retained values. Cover the shared helper's source, +override, projection, and cloning callers rather than fixing only one entry point. + +## 4. Callback Contexts Remain Active After Panic Unwinding + +Status: open. Priority: low. Category: callback-lifetime contract. + +**Relevant code.** `runResolution` in +[resolution.go](../../internal/engine/resolution.go), lines 283–291, and +`runValidation`/`reportVersion` in +[validation.go](../../internal/engine/validation.go), lines 99–106 and 250–257, +set `state.active = false` only after the application callback returns normally. +The root facade forwards the context without an additional lifetime guard. + +**Reproduction.** Retain the supplied context in a resolver, semantic validator, +or version checker, then panic in that callback. Recover the application panic +outside `Load` and invoke the retained context. Public-API probes showed that +`Resolution.IsExplicit` still returns a value and `Validation.Warning` still +accepts a report for both validation and version contexts; none produces the +documented closed-context panic. + +**Expected behavior.** The +[resolution](../config.md#recorded-application-resolution) and +[validation](../config.md#semantic-validation) contracts make these contexts +callback-scoped and require retained contexts to reject use after callback exit. +Application panics remain programming failures and should propagate unchanged; +closing the context during unwinding does not require ConfigKit to recover them +or return a partial result. This finding concerns only the lifetime guard after +an application chooses to recover its own panic. + +**Potential remediation.** Deactivate the context with a defer scoped to each +callback invocation, retaining immediate deactivation on normal return as well +as on panic unwinding. Do not leave the context active through later projection +or metadata work, and do not convert or suppress the callback panic. Apply the +same lifetime handling to resolution, semantic validation, and version checks.