From e5870453f994ab18c171dd9f8515fef25bfe13bc Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 22 Sep 2026 11:40:25 +0000 Subject: [PATCH] Close gaps in public boundary checks --- docs/roadmap/remediation.md | 86 +++++++++++++++++++++++++++++++++++++ public_boundary_test.go | 83 ++++++++++++++++++++++++++++------- 2 files changed, 153 insertions(+), 16 deletions(-) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..fec614e --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,86 @@ +# Public Facade Review Remediation + +## Scope + +Review of implementation commits after baseline `7a858c2` through `06251e3`, +against the [feature roadmap](public-facade.md) and +[implementation plan](implementation.md). + +The production ownership split is in place. No production behavior regression +or public API change was identified in the reviewed diff. One required +architecture-test gap was identified and has now been addressed; it did not +establish an existing public type leak in Promptkit. + +## Finding 1: Complete Reachable Public Type Inspection (P2) + +Status: addressed. + +Location: [public boundary checker](../../public_boundary_test.go), +`publicSurfaceViolations`, in its named/alias and method-set traversal. + +The original checker inspected method sets only for exported package-scope type +names. It did not inspect methods on an unexported root type reached through an +exported function or field. Such methods are still callable by consumers. It +also walked generic parameter constraints but not instantiated type arguments; +an internal argument can escape without appearing in the underlying fields. + +Both examples below were type-checked through the existing fixture helper and +incorrectly produced zero violations: + +```go +// Example 1: a caller can invoke New().Leak(). +type result struct{} +func (result) Leak() hidden.Value { return hidden.Value{} } +func New() result { return result{} } +``` + +```go +// Example 2: an internal type appears in an exported result signature even +// though Box has no field using its type parameter. +type Box[T any] struct{} +func New() Box[hidden.Value] { return Box[hidden.Value]{} } +``` + +Here `hidden` is the fixture's own-module internal package. These are separate +fixture packages, not additions to Promptkit's API. Temporary diagnostic tests +confirmed both false negatives and were removed after review. + +### Resolution + +The recursive named-type traversal now checks reachable root types' value and +pointer method sets, using instantiated selections for their signatures. Named +types and aliases have their concrete type arguments inspected before their +underlying representation is considered. The separate package-scope method +loop was removed. Regression fixtures cover returned private types, private +field types with public pointer methods, generic arguments, aliases that erase +their arguments, and allowed public arguments with recursive method signatures. + +### Acceptance Criteria + +Extend the existing recursive traversal rather than adding a second checker: + +- Inspect accessible methods of reachable root named types, including + unexported types, with the existing cycle protection and promoted-method + checks. Cover pointer and value method sets where applicable. +- Inspect concrete generic type arguments on named types and aliases before + losing them through unaliasing or skipping an external named type's internals. + Continue checking constraints. Do not recursively police unrelated dependency + implementation details. +- Add the two reproductions to the compact fixture table and exercise an + allowed public type argument as a control. Preserve acceptance of private + internal fields and sealed options. Avoid duplicated method traversal. + +Acceptance: both examples are rejected, legitimate private state still passes, +and the full maintainer workflow succeeds. No production API changes are needed. + +## Review Validation + +The existing full maintainer workflow passed: ordinary and race tests, vet, +build, both offline examples, formatting, Markdown links, and repository +hygiene. The targeted diagnostic tests failed as expected, demonstrating the +checker gap rather than a runtime failure. No production or permanent test +files were changed during this review. + +After remediation, the focused checker tests and full maintainer workflow also +passed. The changes are confined to the architecture test and this review log; +production code and public contracts remain unchanged. diff --git a/public_boundary_test.go b/public_boundary_test.go index a148920..44f0c74 100644 --- a/public_boundary_test.go +++ b/public_boundary_test.go @@ -63,6 +63,55 @@ import "example.test/facade/internal/hidden" type privateState struct { value hidden.Value } type Public struct { state privateState } type Option interface { apply(*privateState) } +`, + }, + { + name: "returned private type method is rejected", + source: `package facade +import "example.test/facade/internal/hidden" +type result struct{} +func (result) Leak() hidden.Value { return hidden.Value{} } +func New() result { return result{} } +`, + wantReject: true, + }, + { + name: "reachable private pointer method is rejected", + source: `package facade +import "example.test/facade/internal/hidden" +type result struct{} +func (*result) Leak() hidden.Value { return hidden.Value{} } +type Public struct { Result result } +`, + wantReject: true, + }, + { + name: "internal generic argument is rejected", + source: `package facade +import "example.test/facade/internal/hidden" +type Box[T any] struct{} +func New() Box[hidden.Value] { return Box[hidden.Value]{} } +`, + wantReject: true, + }, + { + name: "internal alias argument is rejected", + source: `package facade +import "example.test/facade/internal/hidden" +type Box[T any] = struct{} +func New() Box[hidden.Value] { return Box[hidden.Value]{} } +`, + wantReject: true, + }, + { + name: "public generic argument and recursive methods are allowed", + source: `package facade +type Value struct{} +type Box[T any] struct{} +func New() Box[Value] { return Box[Value]{} } +type result struct{} +func NewResult() result { return result{} } +func (*result) Next() *result { return nil } `, }, { @@ -178,6 +227,8 @@ func publicSurfaceViolations(pkg *types.Package, modulePath string) []string { var inspect func(types.Type, string) var inspectTuple func(*types.Tuple, string) var inspectTypeParams func(*types.TypeParamList, string) + var inspectTypeArgs func(*types.TypeList, string) + var inspectMethods func(*types.Named, string) inspect = func(typ types.Type, where string) { if typ == nil || seen[typ] { return @@ -186,6 +237,7 @@ func publicSurfaceViolations(pkg *types.Package, modulePath string) []string { switch value := typ.(type) { case *types.Named: + inspectTypeArgs(value.TypeArgs(), where) if isInternalPackage(value.Obj().Pkg(), modulePath) { record(where, value) return @@ -193,8 +245,10 @@ func publicSurfaceViolations(pkg *types.Package, modulePath string) []string { if value.Obj().Pkg() == pkg { inspectTypeParams(value.TypeParams(), where) inspect(value.Underlying(), where) + inspectMethods(value, where) } case *types.Alias: + inspectTypeArgs(value.TypeArgs(), where) if isInternalPackage(value.Obj().Pkg(), modulePath) { record(where, value) return @@ -258,21 +312,12 @@ func publicSurfaceViolations(pkg *types.Package, modulePath string) []string { inspect(params.At(index), where) } } - - for _, name := range pkg.Scope().Names() { - if !token.IsExported(name) { - continue - } - object := pkg.Scope().Lookup(name) - inspect(object.Type(), name) - typeName, ok := object.(*types.TypeName) - if !ok { - continue - } - named, ok := types.Unalias(typeName.Type()).(*types.Named) - if !ok { - continue + inspectTypeArgs = func(args *types.TypeList, where string) { + for index := 0; index < args.Len(); index++ { + inspect(args.At(index), where) } + } + inspectMethods = func(named *types.Named, where string) { for _, methodSet := range []*types.MethodSet{types.NewMethodSet(named), types.NewMethodSet(types.NewPointer(named))} { for index := 0; index < methodSet.Len(); index++ { selection := methodSet.At(index) @@ -281,13 +326,19 @@ func publicSurfaceViolations(pkg *types.Package, modulePath string) []string { continue } if isInternalPackage(method.Pkg(), modulePath) { - record(name+"."+method.Name(), method.Type()) + record(where+"."+method.Name(), selection.Type()) continue } - inspect(method.Type(), name+"."+method.Name()) + inspect(selection.Type(), where+"."+method.Name()) } } } + + for _, name := range pkg.Scope().Names() { + if token.IsExported(name) { + inspect(pkg.Scope().Lookup(name).Type(), name) + } + } sort.Strings(violations) return compactStrings(violations) }