Record API contract audit findings
This commit is contained in:
197
docs/roadmap/interfaces.md
Normal file
197
docs/roadmap/interfaces.md
Normal file
@@ -0,0 +1,197 @@
|
||||
# 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.
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user