202 lines
12 KiB
Markdown
202 lines
12 KiB
Markdown
# Additional Public API Review
|
||
|
||
Reviewed revision: `3ebb848eadf6e61275ecec6e35838ad6576cdd3a`.
|
||
Status: N1–N6 accepted, with N4 incorporated into D6 and N5 into A1.
|
||
Implementation has not begun.
|
||
|
||
## 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, A1, and N1–N6 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` (accepted) | Rename to `SearchPaths`; decision recorded in [api.md](api.md). | Go function rename. |
|
||
| N2: main-tier provenance (accepted) | Rename `LayerBase` to `LayerMain`, with value `"main"`; decision recorded in [api.md](api.md). | Go constant rename and observable string change. |
|
||
| N3: shared-history quota (accepted) | Rename `CodeExplainLimitExceeded` to `CodeHistoryLimitExceeded`, with value `"history.limit_exceeded"`; decision recorded in [api.md](api.md). | Go constant rename and machine-readable code change. |
|
||
| N4: recording-kind diagnostic (accepted through D6) | Incorporated into [D6 in api.md](api.md#recording-methods-d6). | Conditional constant removal. |
|
||
| N5: failed-load source lookup (accepted through A1) | Incorporated into [A1 in api.md](api.md#direct-source-lookup-a1). | Compatible addition. |
|
||
| N6: import-default field names (accepted) | Rename to `FollowSymlinks` and `IncludeHidden`; decision recorded in [api.md](api.md). | Go field renames; YAML keys unchanged. |
|
||
|
||
These accepted design changes are not defect severities or additional release
|
||
gates. The string-value changes in N2 and N3 are explicitly accepted.
|
||
|
||
## N1. Distinguish Discovery Paths From Semantic Paths
|
||
|
||
Status: accepted. [api.md](api.md) owns the selected change; the discussion below
|
||
preserves its rationale and alternatives.
|
||
|
||
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
|
||
|
||
Status: accepted, including the string-value change. [api.md](api.md) owns the
|
||
selected change; the discussion below preserves its rationale and alternatives.
|
||
|
||
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
|
||
|
||
Status: accepted, including the diagnostic-code string change. [api.md](api.md)
|
||
owns the selected change; the discussion below preserves its rationale and alternatives.
|
||
|
||
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
|
||
|
||
Status: accepted and incorporated into [D6](api.md#recording-methods-d6). The
|
||
discussion below preserves the rationale and alternative.
|
||
|
||
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
|
||
|
||
Status: accepted and incorporated into [A1](api.md#direct-source-lookup-a1). The
|
||
discussion below preserves the rationale and alternative.
|
||
|
||
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
|
||
|
||
Status: accepted. [api.md](api.md) owns the selected change; the discussion below
|
||
preserves its rationale and alternative.
|
||
|
||
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
|
||
|
||
All six findings have been accepted and incorporated into [api.md](api.md), with
|
||
N4 folded into D6 and N5 into A1. This audit retains the review evidence and
|
||
rationale; it 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.
|