192 lines
9.2 KiB
Markdown
192 lines
9.2 KiB
Markdown
# Public facade remediation
|
||
|
||
Review baseline: `5dc7e18`. Reviewed commits `f81def9` through `5dc7e18` against
|
||
`eb7de8e`, the agreed [roadmap](public.md), and [implementation plan](implementation.md).
|
||
Status: one open finding; F1–F3 resolved. Review evidence below describes the
|
||
original baseline; resolution records describe the subsequent fixes.
|
||
|
||
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`.
|
||
|
||
## 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
|
||
|
||
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.
|
||
|
||
Reproduced through the public API with a struct containing a `version` string,
|
||
a counting Reader over `version: v1`, and this option:
|
||
|
||
```go
|
||
configkit.Options[Config]{
|
||
Definition: configkit.Definition[Config]{
|
||
Version: &configkit.VersionRule{Path: configkit.Keys("version")},
|
||
},
|
||
}
|
||
```
|
||
|
||
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.
|
||
|
||
### Required correction and validation
|
||
|
||
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.
|
||
|
||
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.
|
||
|
||
## F2 — Close the public-boundary check's type-resolution gaps
|
||
|
||
Priority: medium. Status: resolved.
|
||
|
||
Resolution: [public_boundary_test.go](../../public_boundary_test.go) loads the
|
||
active build's compiler export data and walks resolved Go types, exported method
|
||
sets, and generic constraints. Declaration fixtures cover exposure and valid
|
||
private-backed wrappers. The original three probe declarations now each fail
|
||
the actual root check; the temporary probe was removed afterwards. The everyday
|
||
check passed with Go 1.26.7, and boundary tests passed with Go 1.25.5 and Go 1.26.7
|
||
on Linux. No production behavior or public surface was changed.
|
||
|
||
### Evidence and impact
|
||
|
||
`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.
|
||
|
||
In a disposable archive of `5dc7e18`, these production declarations all compiled
|
||
and `go test . -run '^TestPublicFacadeBoundary$' -count=1` still passed:
|
||
|
||
```go
|
||
type ReviewPaths []engine.Path
|
||
type reviewHidden = engine.Path
|
||
func ReviewLeak() reviewHidden { return engine.Path{} }
|
||
type ReviewGeneric[T interface{ ~[]engine.Path }] struct{}
|
||
```
|
||
|
||
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: resolved.
|
||
|
||
Resolution: restored the baseline comments on root source, path, secret, and
|
||
diagnostic declarations. The complete `go doc -all .` output now matches the
|
||
pre-refactor `eb7de8e` reference, with no missing declarations or contract prose.
|
||
Documentation-link and whitespace checks passed. No behavior or new guarantee
|
||
was introduced.
|
||
|
||
### 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.
|