diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index bc23f10..cfed797 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -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. diff --git a/facade_version_test.go b/facade_version_test.go new file mode 100644 index 0000000..9be1fe7 --- /dev/null +++ b/facade_version_test.go @@ -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) + } +} diff --git a/lifecycle.go b/lifecycle.go index 1808e14..b3fa49a 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -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)) }