# Public facade remediation

Review baseline: `5dc7e18`. Reviewed commits `f81def9` through `5dc7e18` against
`eb7de8e`, the agreed [roadmap](public.md), and [implementation plan](implementation.md).
Status: all four findings 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: resolved.

Resolution: removed the duplicate tail and stray table terminator, escaped the
boundary command's alternation pipes, and linked the new public-boundary test
owner. Content/structure review confirms one task table and one fuzz section.
The documented boundary command, documentation links, and whitespace checks pass.

### 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 did not waive these findings. The 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 during review.

## Remediation completion

All four corrections are implemented and committed as coherent units. Final
validation passed on Linux: the everyday check and full race suite on Go 1.26.7,
and `go test ./...` on Go 1.25.5. The full public Go reference matches `eb7de8e`;
the new version regression was verified failing before its fix; and the original
boundary probes now fail the real boundary check and are not retained in production.
No release was prepared, and pre-existing knowledge-graph changes are not included
in remediation commits.