eric/ConfigKit
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Preserve missing version checker validation

Browse Source
This commit is contained in:
Eric Rakestraw
2026-09-12 17:20:40 +00:00
parent 5dc7e18d83
commit 7395f54513
3 changed files with 230 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

218
docs/roadmap/remediation.md
View File

@@ -1,79 +1,177 @@
# Resolution provenance remediation
# Public facade remediation
Review baseline: `715364b`. Reviewed implementation commits `88f21d0` through
`715364b` against `818d36f`. Status: finding resolved.
Review baseline: `5dc7e18`. Reviewed commits `f81def9` through `5dc7e18` against
`eb7de8e`, the agreed [roadmap](public.md), and [implementation plan](implementation.md).
Status: three open findings; F1 resolved. Review evidence below describes the
original baseline; resolution records describe the subsequent fixes.
## R1 — Avoid copying unused history into the resolution entry view
The previous provenance finding remains resolved; its record is preserved at
`5dc7e18:docs/roadmap/remediation.md`. The field-only resolution-entry snapshot
construction remains intact in `internal/engine/load_metadata.go`.
Status: resolved. Priority: medium.
## F1 — Preserve a nil schema-version checker during option conversion
Priority: high. Status: resolved.
Resolution: option conversion preserves nil Check and installs a wrapper only for
a real checker. The public regression failed with the original panic before the
fix, then passed with typed rejection, no reads/callbacks, and successful source
reuse with a valid checker. Focused facade/version tests, the everyday check,
and the full race suite passed on Linux with Go 1.26.7.
### Evidence and impact
At the review baseline, `newResolutionInput` in [load_metadata.go](../../internal/engine/load_metadata.go) called
`cloneMetadata(metadata)` before adding its effective-path index. `cloneMetadata`
deep-copies `metadata.history`, including each `Change` and its nested records,
as well as any historical-path index. At this lifecycle boundary,
`metadataFromDecoded` and `applyOverlayMetadata` have already populated history.
In [lifecycle.go](../../lifecycle.go), `toEngineDefinition` (lines 233–237 at the
review baseline) installs a non-nil Check closure whenever Version is non-nil,
even if the supplied VersionRule.Check is nil. This defeats the existing
nil-checker rejection in `internal/engine/options.go`. Loading then consumes input
and invokes a nil function through the closure, panicking.
The entry view only exposes `Metadata.Lookup` and source-ID lookup. Neither query
uses history. Every load with a resolver therefore duplicates historical records
that are unnecessary for the requested entry-state queries, including records for
displaced/default paths that the view cannot query. This adds allocation and copy
work proportional to retained history even for a resolver that never calls the
new methods. No wall-clock or memory threshold was measured or is asserted here.
Reproduced through the public API with a struct containing a `version` string,
a counting Reader over `version: v1`, and this option:
The historical completion record at
`28e7fc5:docs/roadmap/implementation.md` also states that the view "does not
retain history"; the constructor contradicts that claim.
```go
configkit.Options[Config]{
Definition: configkit.Definition[Config]{
Version: &configkit.VersionRule{Path: configkit.Keys("version")},
},
}
```
### Correction
At `eb7de8e`, this returns `*configkit.LoadError` with `CodeOptionsInvalid` and
zero reader calls. At `5dc7e18`, it reads the source and panics. Reading also claims
the single-use source, so recovering does not restore the old behavior. This is
a regression in established validation and failure ordering, not a proposed new
restriction on version rules.
Construct the entry view from the effective field/path information and source
records needed by its queries, avoiding the unused deep copy of history. A small
field-only copy using the existing record clone helpers is one straightforward
solution. Immutable sharing or another ownership arrangement is also acceptable
if it preserves the agreed stable queries and detached public returns.
### Required correction and validation
Do not change `cloneMetadata` globally to drop history: final result metadata
still requires it. Do not alter public APIs, weaken detachment, introduce resource
caps, or make a particular representation/complexity bound a public guarantee.
Correct the completion record to describe the verified implementation.
Convert Version and Path while preserving a nil Check. Install the forwarding
closure only when the supplied Check is non-nil. Leave validation and diagnostic
order in the engine; do not add facade validation or recover-and-reclassify.
This finding concerns demonstrably unused work, not a requirement for eager
copying, zero allocations, or a new security restriction.
Add a public-root regression test asserting typed `CodeOptionsInvalid`, no result,
no source consumption, and no application callback execution. Reusing the same
Reader source with a valid rule should still succeed. Retain valid checker
coverage for version/context conversion. Run root facade/version tests, the
everyday check, and the race suite.
### Acceptance and validation
## F2 — Close the public-boundary check's type-resolution gaps
- Inspection of the entry-view construction confirms it no longer deep-copies
history solely for these queries.
- Existing public tests still prove fixed entry information during normalization,
moves, and deletions; detached field/source returns; and correct final history.
- Update the completion record only after verifying its claims. Existing behavioral
tests are sufficient unless the chosen fix exposes a new risk; do not freeze a
private struct layout in a test just to prove this optimization.
- Run `go test . -run 'Resolution|Metadata|Provenance|Historical|Explain'`,
`./scripts/check.sh`, and `go test -race ./...`. Use the full release checker
when preparing a candidate under the release procedure.
Priority: medium. Status: open.
## Review results and boundaries
### Evidence and impact
Resolution: the entry constructor now copies only effective `FieldInfo` records
using `cloneFieldInfo`, builds the existing effective-path index, and retains its
detached source records. It no longer invokes `cloneMetadata`; final-result
metadata cloning and history are unchanged. The Stage 5 completion record has
been corrected. No public API, resource limit, or security restriction was added.
`TestPublicFacadeBoundary` in
[import_boundary_test.go](../../import_boundary_test.go) checks selector syntax in
function signatures and public struct fields, but skips non-struct type bodies
and does not resolve local type names or inspect type-declaration constraints.
No current public engine-type leak was identified; the gap is in the guard meant
to prevent future accidental exposure.
Post-fix validation passed on Linux with Go 1.26.7: the focused resolution,
metadata, provenance, historical, and explanation tests; `./scripts/check.sh`;
and `go test -race ./...`.
In a disposable archive of `5dc7e18`, these production declarations all compiled
and `go test . -run '^TestPublicFacadeBoundary$' -count=1` still passed:
The full Linux release checker passed on `715364b` using Go 1.25.5 and Go 1.26.7,
including race tests, consumer examples, documentation links, and cross-builds.
No functional provenance, detachment, credential-timing, or application-policy
restriction regression was identified in this review. This is not native macOS
release evidence or publication approval.
```go
type ReviewPaths []engine.Path
type reviewHidden = engine.Path
func ReviewLeak() reviewHidden { return engine.Path{} }
type ReviewGeneric[T interface{ ~[]engine.Path }] struct{}
```
The warning-and-retain fallback in the maintained Notarius-shaped example is
explicitly documented as application policy, not a new library requirement.
The existing callback-lifetime and redaction contracts remain authoritative.
No production code or existing tests were changed during review.
The collection exposes engine values; the private alias hides an engine return
type from the scanner; the constraint exposes an internal type despite an empty
struct body. These are ordinary declarations that can create exactly the
accidental public commitments the selected wrapper boundary is meant to prevent.
### Required correction and validation
Resolve types across root declarations and inspect exported/reachable type
surfaces, including named collection/function/interface types and generic
constraints. Prefer a small type-aware implementation using standard Go tooling
over isolated selector-pattern exceptions. Keep private backing fields legal;
their layout is not public API. Do not add runtime restrictions or broaden the
check into a general security scanner.
Use focused positive/negative declaration fixtures covering the reproduced misses,
existing direct-exposure cases, and legitimate wrappers, public records, and
generic public types. Fixtures need not enter production. Run boundary tests and
the everyday check.
## F3 — Restore consumer contracts on public Go declarations
Priority: medium. Status: open.
### Evidence and impact
Comparing `go doc -all .` at `eb7de8e` and `5dc7e18` found unchanged exported
declaration signatures but lost substantive reference documentation. Much of that
text now remains only on internal engine declarations:
- [source.go](../../source.go): Reader omits that ConfigKit never closes the
reader; Bytes omits label requirements and deferred constructor errors;
NoSource omits that it creates no source record.
- [diagnostic.go](../../diagnostic.go): LoadError loses its non-unwrapping contract;
Error, Diagnostics, and Sources have no reference comments; WriteDiagnostics
omits control-character escaping and writer/short-write behavior.
- [path.go](../../path.go): Path loses concrete semantic/escaping details;
Index loses the negative-index behavior.
- [secret.go](../../secret.go): SecretEnv omits that invalid names are reported
if the reference survives loading.
These are consumer ownership and usage facts, not plumbing to hide with the
engine. Their loss diverges from the documented root-facade goal and the
documentation policy's declaration-level API ownership.
### Required correction and validation
Restore substantive existing comments on root declarations, linking to the
configuration contract for shared semantics where appropriate. Review the complete
reference diff, not just these examples. Do not invent stronger guarantees or
require consumers to inspect engine documentation. Internal comments can explain
mechanics without becoming a competing public contract.
Validate with `go doc -all .`, documentation-link checks, and content review;
no golden full-documentation snapshot is required.
## F4 — Repair the duplicated development-guide tail and table row
Priority: low. Status: open.
### Evidence and impact
[docs/development.md](../development.md), lines 122–134 at the review baseline,
contains a stray backtick/table terminator, a duplicate checker row outside its
table, and a second copy of the Fuzz validation section. The boundary-check
command in the table at line 109 also contains unescaped pipes, splitting the
Markdown row into unintended cells. The link checker passes because these are
rendering/content errors rather than missing targets.
### Required correction and validation
Retain one well-formed task table and one fuzz section with the current engine/YAML
commands. Escape regex alternation pipes in the table consistently with other
rows. Review the rendered structure and run local documentation-link and
whitespace checks. No new documentation framework or test is needed.
## Architecture assessment and validation boundaries
The principal target is implemented: public named wrappers and records remain in
root; substantial machinery lives in one internal engine; document and YAML
boundaries remain separate. The temporary facade and old root engine are gone.
Callbacks wrap engine context pointers, configuration remains the actual application
type, and inspection is frozen non-generic state separate from public Config.
The exact-type secret binding is per-operation, not a public registration API.
Review validation passed on Linux:
- `./scripts/check.sh` with Go 1.26.7 (formatting, whitespace, tests, vet, build);
- `go test -race ./...` with Go 1.26.7;
- `GOTOOLCHAIN=go1.25.5 GOPROXY=off go test ./...`;
- public reference comparison and isolated baseline/current F1 reproduction;
- the isolated F2 negative probe, demonstrating the guard's missed violations.
Passing existing tests does not waive these findings. This review did not repeat
the complete release checker or native macOS validation and is not release
approval. The two knowledge-graph artifact modifications present before review
were left untouched. Only this remediation document was changed in the workspace.

66
facade_version_test.go Normal file
View File

@@ -0,0 +1,66 @@
package configkit_test
import (
"errors"
"strings"
"testing"
configkit "gitea.maximumdirect.net/eric/ConfigKit"
)
type versionCountingReader struct {
*strings.Reader
reads int
}
func (r *versionCountingReader) Read(data []byte) (int, error) {
r.reads++
return r.Reader.Read(data)
}
func TestFacadeRejectsMissingVersionCheckerBeforeReading(t *testing.T) {
t.Parallel()
type configuration struct {
Version string `yaml:"version"`
}
reader := &versionCountingReader{Reader: strings.NewReader("version: v1\n")}
source := configkit.Reader("version.yml", reader)
var defaults, resolutions, validations, checks int
options := configkit.Options[configuration]{Definition: configkit.Definition[configuration]{
Version: &configkit.VersionRule{Path: configkit.Keys("version")},
Defaults: func() configuration { defaults++; return configuration{} },
Resolve: func(*configuration, *configkit.Resolution) error { resolutions++; return nil },
Validate: func(*configuration, *configkit.Validation) { validations++ },
}}
result, err := configkit.Load(source, options)
var loadError *configkit.LoadError
if result != nil || !errors.As(err, &loadError) {
t.Fatalf("missing checker: result = %v, error = %v", result, err)
}
diagnostics := loadError.Diagnostics()
if len(diagnostics) != 1 || diagnostics[0].Code != configkit.CodeOptionsInvalid {
t.Fatalf("diagnostics = %v", diagnostics)
}
if reader.reads != 0 || defaults != 0 || resolutions != 0 || validations != 0 {
t.Fatalf("invalid options performed work: reads=%d defaults=%d resolve=%d validate=%d", reader.reads, defaults, resolutions, validations)
}
options.Definition.Version.Check = func(version configkit.Version, validation *configkit.Validation) error {
checks++
if version != (configkit.Version{Present: true, Text: "v1"}) {
t.Errorf("version = %#v", version)
}
validation.Warning(configkit.Keys("version"), "version accepted")
return nil
}
result, err = configkit.Load(source, options)
if err != nil || result == nil || result.Config.Version != "v1" {
t.Fatalf("reuse with checker: result = %v, error = %v", result, err)
}
if checks != 1 || defaults != 1 || resolutions != 1 || validations != 1 {
t.Fatalf("callback counts: check=%d defaults=%d resolve=%d validate=%d", checks, defaults, resolutions, validations)
}
diagnostics = result.Diagnostics()
if len(diagnostics) != 1 || diagnostics[0].Code != configkit.CodeVersionWarning {
t.Fatalf("version callback diagnostics = %v", diagnostics)
}
}

9
lifecycle.go
View File

@@ -232,9 +232,12 @@ func toEngineDefinition[T any](definition Definition[T]) engine.Definition[T] {
}
if definition.Version != nil {
rule := definition.Version
result.Version = &engine.VersionRule{Path: toEnginePath(rule.Path), Check: func(version engine.Version, validation *engine.Validation) error {
return rule.Check(Version{Present: version.Present, Null: version.Null, Text: version.Text}, &Validation{value: validation})
}}
result.Version = &engine.VersionRule{Path: toEnginePath(rule.Path)}
if rule.Check != nil {
result.Version.Check = func(version engine.Version, validation *engine.Validation) error {
return rule.Check(Version{Present: version.Present, Null: version.Null, Text: version.Text}, &Validation{value: validation})
}
}
}
if definition.Secret != nil {
result.Secret = func(path engine.Path) bool { return definition.Secret(fromEnginePath(path)) }