Files
configkit/docs/roadmap/audit.md

11 KiB
Raw Blame History

Additional Public API Review

Reviewed revision: 3ebb848eadf6e61275ecec6e35838ad6576cdd3a. Status: N1 accepted; N2–N6 remain discussion findings. 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 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 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

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

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

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

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

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

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

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; 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.