12 KiB
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 and independently assesses the public interface through consumer workflows, declaration comments, the configuration contract, 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 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. |
Go function rename. |
| N2: main-tier provenance (accepted) | Rename LayerBase to LayerMain, with value "main"; decision recorded in 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. |
Go constant rename and machine-readable code change. |
| N4: recording-kind diagnostic (accepted through D6) | Incorporated into D6 in api.md. | Conditional constant removal. |
| N5: failed-load source lookup (accepted through A1) | Incorporated into A1 in api.md. | Compatible addition. |
| N6: import-default field names (accepted) | Rename to FollowSymlinks and IncludeHidden; decision recorded in 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 owns the selected change; the discussion below preserves its rationale and alternatives.
Evidence: path.go defines Path, Keys, and ParsePath for
configuration-value coordinates. 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 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 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 owns the selected change; the discussion below preserves its rationale and alternatives.
Evidence: records.go exposes
CodeExplainLimitExceeded = "explain.limit_exceeded". Its production sites include
executeLoadRequest in load.go and
inheritanceHistoryBudgetDiagnostic in
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. The discussion below preserves the rationale and alternative.
Evidence: the production use of CodeResolveInvalidKind is the kind guard in
resolutionState.addRecord in
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. The discussion below preserves the rationale and alternative.
Evidence: 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:
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 owns the selected change; the discussion below preserves its rationale and alternative.
Evidence: 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, 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.