From 14c83a651508539c21866ab4229d854f259abc70 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 27 Oct 2026 14:21:01 +0000 Subject: [PATCH] Record additional public API review opportunities --- docs/roadmap/audit.md | 185 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 185 insertions(+) create mode 100644 docs/roadmap/audit.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..9814c2a --- /dev/null +++ b/docs/roadmap/audit.md @@ -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.