202 lines
9.8 KiB
Markdown
202 lines
9.8 KiB
Markdown
# API and Interface Contract Audit
|
||
|
||
Reviewed baseline: `01b08e76a3fecc22c4414437bfeaa07b4b7a85aa`.
|
||
|
||
Status: four open findings. This is an audit record, not an implementation plan.
|
||
No production code or repository tests were changed.
|
||
|
||
The [interface remediation roadmap](interface-remediation.md) describes the
|
||
proposed target, required changes, dependencies, and remaining design decision.
|
||
This audit retains the findings and reproduction evidence.
|
||
|
||
## Scope and evidence
|
||
|
||
Reviewed the root-owned public facade and its conversions, engine loading and
|
||
callback boundaries, source/discovery and filesystem ownership, schema and typed
|
||
value handling, metadata/history, safe rendering and comparison, secrets, and
|
||
the document/YAML boundary against declaration comments, the
|
||
[configuration contract](../config.md), and applicable architectural decisions.
|
||
The root delegates to the synchronous engine lifecycle; parser-independent
|
||
documents support source assembly and inheritance, and completed results retain
|
||
detached configuration and frozen inspection state.
|
||
|
||
The findings below were reproduced through the public API with a disposable Go
|
||
program outside the repository, using labeled byte sources and `fstest.MapFS`.
|
||
The full existing suite passed with Go 1.26.10 on Linux:
|
||
|
||
```sh
|
||
GOTOOLCHAIN=local GOTELEMETRY=off GOPROXY=off go test ./... -count=1
|
||
```
|
||
|
||
The existing tests cover many ownership, lifetime, error, and zero-value cases,
|
||
but do not detect these cross-interface discrepancies. No benchmarks, external
|
||
service calls, or new test files were needed. The recommendations restore
|
||
existing contracts rather than introduce new API restrictions or guarantees.
|
||
|
||
## 1. Resolution presence queries confuse explicit keys with sequence indexes
|
||
|
||
Priority: P2. Status: open.
|
||
|
||
Relevant code:
|
||
|
||
- [`Resolution.IsExplicit`](../../internal/engine/resolution.go), lines 75–82.
|
||
- `Resolution.InputInfo` and `resolutionInput.lookup` in
|
||
[resolution.go](../../internal/engine/resolution.go) and
|
||
[load_metadata.go](../../internal/engine/load_metadata.go).
|
||
- `resolveSchemaPath` in [schema.go](../../internal/engine/schema.go).
|
||
|
||
The public [path contract](../../path.go) distinguishes explicit mapping keys
|
||
from sequence indexes. The [resolution contract](../config.md#recorded-application-resolution)
|
||
defines surviving-input presence and a fixed entry-state metadata view;
|
||
[metadata queries](../config.md#layered-metadata) preserve container shapes.
|
||
|
||
`IsExplicit` validates against the schema but then looks up presence using only
|
||
`path.String()`. In dynamic `any` state, schema validation cannot establish the
|
||
actual container shape. Consequently these distinct queries collide:
|
||
|
||
```go
|
||
key := configkit.Keys("shape", "0")
|
||
index := configkit.Keys("shape").Index(0)
|
||
```
|
||
|
||
Load `map[string]any` from `shape: [input]` with a resolver that queries both
|
||
paths, without changing the configuration. Observed results:
|
||
|
||
| Query | `Resolution.IsExplicit` | `Resolution.InputInfo` found | Final `Metadata.IsExplicit` |
|
||
| --- | --- | --- | --- |
|
||
| Explicit mapping key | true | false | false |
|
||
| Explicit sequence index | true | true | true |
|
||
|
||
The mapping-key path never existed and should not be reported as explicit.
|
||
Applications branching on presence receive inconsistent answers depending on
|
||
which public query they use, even during an otherwise unchanged load.
|
||
|
||
Potential remediation: use the retained entry-state semantic shape when checking
|
||
resolution presence, consistently with `InputInfo` and `Metadata.IsExplicit`.
|
||
Review the same string-only presence lookup in `Default` while preserving its
|
||
existing assignment and callback-lifetime rules. Add focused public coverage for
|
||
both mapping-to-index and index-to-mapping mismatches under dynamic values,
|
||
including contextually parsed pointers. This does not require changing the
|
||
documented treatment of invalid schema paths.
|
||
|
||
## 2. Resolution can transfer explicitness to a different semantic path
|
||
|
||
Priority: P2. Status: open.
|
||
|
||
Relevant code: `applyResolutionMetadata`, `applyResolutionFieldOrigins`, and
|
||
`refreshResolutionFields` in
|
||
[load_metadata.go](../../internal/engine/load_metadata.go), especially lines
|
||
371–387; final shape checking in
|
||
[`Metadata.effectiveField`](../../internal/engine/metadata.go).
|
||
|
||
The [metadata contract](../config.md#layered-metadata) distinguishes key/index
|
||
shapes and keeps derived values for absent input non-explicit. Recorded
|
||
resolution changes origins without creating input explicitness.
|
||
|
||
Reproduction:
|
||
|
||
1. Load `map[string]any` from `shape: {'0': old}`.
|
||
2. In resolution, replace `configuration["shape"]` with `[]any{"new"}` and
|
||
record normalization at `Keys("shape")`.
|
||
3. Query final metadata using the two paths from finding 1.
|
||
|
||
The old explicit mapping-key path correctly stops being effective, but the newly
|
||
created sequence index reports `Explicit: true`. That sequence index was never
|
||
supplied by input. `refreshResolutionFields` reuses the old `FieldInfo` whenever
|
||
its textual pointer survives; it updates null state without checking whether
|
||
the old and new containers give that pointer different semantic shapes. Final
|
||
lookup validates only the new shape, so it cannot recover the lost distinction.
|
||
|
||
This can mislead consumers inspecting whether a final value was explicitly
|
||
configured rather than introduced by application resolution. The converse
|
||
sequence-to-numeric-key transition has the same underlying collision.
|
||
|
||
Potential remediation: retain entry/effective shape evidence across resolution
|
||
and carry input explicitness forward only for the matching semantic path.
|
||
Preserve existing explicitness for same-shape paths and the recorded resolution
|
||
origin; do not clear every descendant's presence merely because an ancestor was
|
||
normalized. Cover both shape transitions and agreement among `Lookup`,
|
||
`IsExplicit`, and explanation. Fixing finding 1's callback query alone does not
|
||
repair this publication path.
|
||
|
||
## 3. Metadata history mixes numeric-key and sequence-index definitions
|
||
|
||
Priority: P2. Status: open.
|
||
|
||
Relevant code:
|
||
|
||
- [`Metadata.History`](../../internal/engine/metadata.go), lines 56–65.
|
||
- `metadataFromDecoded`, `applyOverlayMetadata`, and history event construction
|
||
in [load_metadata.go](../../internal/engine/load_metadata.go).
|
||
- Shape-aware replay in [`Inspection.Explain`](../../internal/engine/explain.go).
|
||
|
||
The [metadata contract](../config.md#layered-metadata) promises semantic
|
||
key/index distinctions for historical as well as effective queries. The
|
||
historical-path index retains both shapes, but `History` uses that index only to
|
||
check that the query was ever known. It then retrieves the entire history bucket
|
||
by textual pointer, without selecting the query's shape.
|
||
|
||
Reproduction: load `shape: {'0': old}` into `map[string]any`, then apply a CLI
|
||
override at `Keys("shape")` with `[]any{"new"}`. Both explicit shapes were
|
||
observed and share `/shape/0`.
|
||
|
||
`History(Keys("shape").Index(0))` incorrectly includes the original base
|
||
definition of the mapping key, and identifies that base origin as the previous
|
||
origin of the CLI operation. The old input defined a key, not an index. In the
|
||
same successful result, `Explain` for the explicit index correctly shows only
|
||
its CLI introduction. `History` for the key and index returns identical records
|
||
despite their different lifetimes.
|
||
|
||
Consequences include false source attribution and disagreement between the two
|
||
public history interfaces when consumers inspect removed or introduced paths.
|
||
|
||
Potential remediation: retain enough container-shape evidence with stored
|
||
history events, or select relevant events from existing shape-aware replay
|
||
evidence. Filter exact definitions and previous origins by the queried semantic
|
||
shape while retaining genuine ancestor replacement/removal events. Filtering
|
||
only on the current `Change.Path` is insufficient where construction already
|
||
replaced concrete segment kinds with parsed tokens. Preserve the intentional
|
||
one-pointer-per-entry `HistoricalPaths` enumeration and contextual behavior of
|
||
`ParsePath`; neither public contract needs redesign. Cover both direction changes
|
||
and compare metadata history with explanation.
|
||
|
||
## 4. Diagnostic canonicalization breaks composition claimant ordering
|
||
|
||
Priority: P3. Status: open.
|
||
|
||
Relevant code: `compositionConflictDiagnostic` in
|
||
[compose.go](../../internal/engine/compose.go) and `canonicalDiagnostics` in
|
||
[diagnostic.go](../../internal/engine/diagnostic.go), particularly lines 88–92.
|
||
The latter runs both while constructing `LoadError` and in `WriteDiagnostics`.
|
||
|
||
The [strict-composition contract](../config.md#strict-document-composition)
|
||
specifies the first claim in scoped assembly order as primary and every remaining
|
||
claim in the same order as related locations.
|
||
|
||
A native-filesystem fixture demonstrates the discrepancy:
|
||
|
||
```text
|
||
main.yml imports a.yml, b.yml, c.yml
|
||
a.yml imports z.yml
|
||
b.yml, c.yml, and z.yml each define x
|
||
```
|
||
|
||
Acquisition assigns main=source-1, a=source-2, z=source-3, b=source-4,
|
||
c=source-5. Dependency-ready assembly considers the three conflicting claims
|
||
in b, c, z order. The composer correctly chooses b as primary and constructs
|
||
related locations c, z. Generic canonicalization sorts those related locations
|
||
by acquisition ID, publishing z, c instead. The reproduced public diagnostic
|
||
has primary source-4 and related source-3, source-5.
|
||
|
||
All claimants remain present and the conflict still fails loading, so this is
|
||
lower impact than the presence/history defects. It nevertheless violates the
|
||
documented reporting order and gives consumers two different ordering rules
|
||
within one conflict report.
|
||
|
||
Potential remediation: preserve the selected semantic ordering for composition
|
||
claimant lists through accumulation and rendering, while retaining canonical
|
||
ordering of diagnostic records and the established sorting of related locations
|
||
for diagnostics without a semantic-order contract. Add one public failure-path
|
||
case whose acquisition and claimant orders differ and verify both structured
|
||
diagnostics and rendered output. No scheduler or precedence change is needed.
|