87 lines
3.9 KiB
Markdown
87 lines
3.9 KiB
Markdown
# 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.
|