Close gaps in public boundary checks
This commit is contained in:
86
docs/roadmap/remediation.md
Normal file
86
docs/roadmap/remediation.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user