Record functional correctness audit findings

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.