Record additional public API review opportunities
This commit is contained in:
185
docs/roadmap/audit.md
Normal file
185
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,185 @@
|
||||
# Additional Public API Review
|
||||
|
||||
Reviewed revision: `3ebb848eadf6e61275ecec6e35838ad6576cdd3a`.
|
||||
Status: discussion findings; none of the additional changes below is selected
|
||||
or implemented.
|
||||
|
||||
## Scope And Assessment
|
||||
|
||||
This review accepts the inventory in [review.md](review.md) and independently
|
||||
assesses the public interface through consumer workflows, declaration comments,
|
||||
the [configuration contract](../config.md), and targeted implementation tracing.
|
||||
It covers loading/discovery, source and resource ownership, paths and collection
|
||||
registration, callbacks, inspection/diffs, secrets, and diagnostics. It does not
|
||||
repeat the symbol inventory or conduct a correctness/performance audit.
|
||||
|
||||
[api.md](api.md) owns the accepted D1–D8 and A1 changes. Those decisions remain
|
||||
settled; public path comparison (A2) remains deferred. The findings here are
|
||||
additional opportunities, including consequences of the accepted cleanup.
|
||||
|
||||
The principal API boundaries remain useful. No broader consolidation of loading
|
||||
options, filesystem roles, callbacks, or the two history projections is warranted
|
||||
by this review. The remaining opportunities concern vocabulary, removal of an
|
||||
obsolete input-error category, and a missing convenience on failed loads.
|
||||
|
||||
| Finding | Recommendation | Compatibility impact if accepted |
|
||||
| --- | --- | --- |
|
||||
| N1: discovery `Paths` | Rename to `SearchPaths`. | Go function rename. |
|
||||
| N2: main-tier provenance | Rename `LayerBase` to `LayerMain`, with value `"main"`. | Go constant rename and observable string change. |
|
||||
| N3: shared-history quota | Rename `CodeExplainLimitExceeded` to `CodeHistoryLimitExceeded`, with value `"history.limit_exceeded"`. | Go constant rename and machine-readable code change. |
|
||||
| N4: recording-kind diagnostic | Retire `CodeResolveInvalidKind` if D6 removes its public trigger. | Conditional constant removal. |
|
||||
| N5: failed-load source lookup | Add `LoadError.Source(id)` alongside accepted A1. | Compatible addition. |
|
||||
| N6: import-default field names | Consider `FollowSymlinks` and `IncludeHidden`; lower priority. | Go field renames; YAML keys unchanged. |
|
||||
|
||||
These are design recommendations, not defect severities or additional release
|
||||
gates. In particular, N2 and N3 require deliberate approval of their string-value
|
||||
changes; accepting other naming cleanup does not authorize them automatically.
|
||||
|
||||
## N1. Distinguish Discovery Paths From Semantic Paths
|
||||
|
||||
Evidence: [path.go](../../path.go) defines `Path`, `Keys`, and `ParsePath` for
|
||||
configuration-value coordinates. [discovery.go](../../discovery.go) defines
|
||||
`Paths(paths ...string) Search` for literal host-file candidates. In the same
|
||||
package, the plural of `Path` therefore constructs a different concept rather
|
||||
than a collection of semantic paths. The preliminary catalog recommends retaining
|
||||
the name and explaining the distinction.
|
||||
|
||||
Recommendation: rename the discovery constructor to
|
||||
`SearchPaths(paths ...string) Search`. This aligns it with `SearchDirectory` and
|
||||
makes a call such as `SearchPaths("./app.yml", "/etc/app.yml")` recognizable
|
||||
without inspecting its return type. Keep literal-path interpretation, candidate
|
||||
order, deferred acquisition, and all search behavior unchanged. No rename of
|
||||
`Path`, new path type, or discovery redesign is needed.
|
||||
|
||||
Alternative: retain `Paths`. It is concise inside `SearchConfig` and its comment
|
||||
is accurate, but package completion and standalone calls still mix filesystem
|
||||
and configuration-path vocabulary. `FileCandidates` is another viable name,
|
||||
although it is less consistent with `SearchDirectory`.
|
||||
|
||||
## N2. Use Main Consistently In Provenance
|
||||
|
||||
Evidence: `LoadLayered(main, ...)`, `SourceInfo.UnderlayIndex`, the configuration
|
||||
contract, and the accepted `SourceVersionModeMainOnly` name distinguish main input
|
||||
from lower-priority underlays. [records.go](../../records.go) instead exposes
|
||||
`LayerBase = "base"` for the main tier. A consumer presenting provenance must
|
||||
learn that this base is above underlays and is distinct from application defaults.
|
||||
The preliminary catalog treats this as a documentation concern.
|
||||
|
||||
Recommendation: use `LayerMain = "main"` so API calls, provenance records, and
|
||||
rendered layer labels use the same term. Preserve the existing layer category and
|
||||
all precedence rules; imported contributions retain their owning tier. This is
|
||||
a vocabulary change, not a new layer or a change to source acquisition order.
|
||||
|
||||
Changing the literal affects applications that compare, serialize, or display
|
||||
layer strings as well as those referring to the Go constant. If selected, migrate
|
||||
those uses and document the change together before v1.
|
||||
|
||||
Alternative: rename only the Go constant and keep `"base"`. That reduces string
|
||||
compatibility impact but preserves two names for the same concept in application
|
||||
code and rendered evidence. Keeping both the existing symbol and value is also
|
||||
reasonable if the modest clarity gain does not justify migration; the preferred
|
||||
pre-v1 outcome is one consistent term.
|
||||
|
||||
## N3. Name The Retained-History Limit For Its Actual Scope
|
||||
|
||||
Evidence: [records.go](../../records.go) exposes
|
||||
`CodeExplainLimitExceeded = "explain.limit_exceeded"`. Its production sites include
|
||||
`executeLoadRequest` in [load.go](../../internal/engine/load.go) and
|
||||
`inheritanceHistoryBudgetDiagnostic` in
|
||||
[inheritance_walk.go](../../internal/engine/inheritance_walk.go). It can fail a load
|
||||
without an application calling `Explain`. The completed shared model supplies
|
||||
metadata, explanation, historical membership, and internal attribution readers.
|
||||
|
||||
Recommendation: rename the constant to `CodeHistoryLimitExceeded` and its value
|
||||
to `"history.limit_exceeded"`. This describes the resource being bounded and
|
||||
avoids suggesting that skipping explanation would avoid the load failure.
|
||||
Preserve the quota, retained-node accounting, diagnostic locations, and admission
|
||||
timing. This does not add a limit setting or change which configurations load.
|
||||
|
||||
Alternative: rename the Go constant but retain the existing code string, or keep
|
||||
both names unchanged and clarify the diagnostic documentation. Either preserves
|
||||
more consumer compatibility, but a retained `"explain.limit_exceeded"` remains
|
||||
misleading to applications that work directly with diagnostic codes. A coordinated
|
||||
pre-v1 change is preferable if the new terminology is accepted.
|
||||
|
||||
## N4. Review The Diagnostic Made Obsolete By D6
|
||||
|
||||
Evidence: the production use of `CodeResolveInvalidKind` is the kind guard in
|
||||
`resolutionState.addRecord` in
|
||||
[resolution.go](../../internal/engine/resolution.go). Today, public `Record` can
|
||||
pass arbitrary `ChangeKind` values into it. Accepted D6 replaces that method with
|
||||
two methods that select supported intents; `Default` and `Normalize` also supply
|
||||
fixed supported kinds. This is distinct from D8's already unproduced
|
||||
`CodeOptionsUnavailable`.
|
||||
|
||||
Recommendation: as part of D6 implementation, confirm that no supported public
|
||||
input can produce `resolve.invalid_kind`, then remove its exported constant.
|
||||
Retain the other recording diagnostics and the inspection `ChangeKind` values.
|
||||
Internal defensive checks can remain where useful; their representation does
|
||||
not require an unreachable public configuration-error category.
|
||||
|
||||
Alternative: retain the constant for consistency with previous diagnostics or
|
||||
a concrete future use. That costs little code but leaves an unexplained public
|
||||
error category after its input mechanism has disappeared. Do not add a public
|
||||
trigger simply to justify keeping it. The removal remains conditional on the
|
||||
final D6 implementation, rather than assuming a guard must be deleted now.
|
||||
|
||||
## N5. Offer Direct Source Lookup On Failed Loads
|
||||
|
||||
Evidence: [diagnostic.go](../../diagnostic.go) exposes `LoadError.Diagnostics()`
|
||||
and `LoadError.Sources()`. Diagnostic primary and related locations refer to
|
||||
source IDs. Accepted A1 adds direct lookup on successful results, while
|
||||
`Resolution.Source` already supports it during callbacks. A custom error reporter
|
||||
still has to scan or index the failed load's source slice to follow the same IDs.
|
||||
|
||||
Recommendation: add this companion to A1:
|
||||
|
||||
```go
|
||||
func (e *LoadError) Source(id string) (SourceInfo, bool)
|
||||
```
|
||||
|
||||
It should return a detached record from the same partial inventory as
|
||||
`LoadError.Sources()`, with false for empty/unknown IDs and nil/zero errors.
|
||||
Preserve the existing failed-load redaction and available evidence; a lookup
|
||||
does not complete acquisition or upgrade a partial destination/version record.
|
||||
This is passive lookup, with no filesystem access or new source-completeness
|
||||
guarantee. Existing source lookup/conversion helpers can be shared privately;
|
||||
no public registry, interface hierarchy, or constant-time guarantee is needed.
|
||||
|
||||
Alternative: keep only `Sources()`. That is sufficient for applications using
|
||||
the standard diagnostic renderer or building an index once. The addition can
|
||||
also wait until after v1, since it is compatible. Adding it with A1 is a small,
|
||||
coherent way to remove the same consumer bookkeeping on both outcomes of a load.
|
||||
|
||||
## N6. Make The Go Import Defaults More Self-Describing
|
||||
|
||||
Evidence: [lifecycle.go](../../lifecycle.go) exposes `ImportDefaults.Follow` and
|
||||
`ImportDefaults.Hidden`. Their comments explain symbolic-link traversal and
|
||||
inclusion of hidden files/directories, but an options literal alone does not
|
||||
name the object being followed or say whether hidden entries are included.
|
||||
|
||||
Recommendation, at lower priority: consider `FollowSymlinks` and `IncludeHidden`
|
||||
for those Go fields. Preserve `*bool`, nil defaulting, explicit false, the current
|
||||
search scope, and the YAML keys `follow` and `hidden`. This would make the public
|
||||
Go configuration more descriptive without changing the import language. Leave
|
||||
`Required`, `Recursive`, and `Pattern` alone; they do not have the same ambiguity
|
||||
in context.
|
||||
|
||||
Alternative: retain the current Go names to mirror YAML exactly. That is a real
|
||||
benefit for readers moving between authored imports and application defaults,
|
||||
and existing comments already resolve the ambiguity. This is an optional naming
|
||||
refinement, weaker than N1's collision between two different path concepts.
|
||||
|
||||
## Discussion And Validation
|
||||
|
||||
Discuss the renames and string-value changes first, then N4 alongside D6 and N5
|
||||
alongside A1. N6 can be decided independently. Accepted additions should be moved
|
||||
into [api.md](api.md); this audit is not an implementation plan or a permanent
|
||||
second API reference.
|
||||
|
||||
The review used the trusted catalog, root declarations and comments, current
|
||||
contracts and consumer guidance, and graph-assisted production-reference checks.
|
||||
No production code, tests, accepted decisions, or deferred features were changed.
|
||||
No independent behavioral audit or performance benchmarks were run. Validation
|
||||
for this document consists of the local documentation-link check and whitespace
|
||||
review; it is not evidence that the proposed API changes are implemented.
|
||||
Reference in New Issue
Block a user