Record source version review findings
This commit is contained in:
@@ -1,4 +1,4 @@
|
||||
# Destination review findings
|
||||
# ConfigKit review findings
|
||||
|
||||
Reviewed `d3d468a..945e111` against the approved
|
||||
[input-destinations roadmap](input-destinations.md),
|
||||
@@ -145,3 +145,205 @@ At `10edd95d4d9060bd46db51e0d6782c12dbda3120`, using
|
||||
- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...`.
|
||||
|
||||
The review changed only this findings document; it did not implement fixes.
|
||||
|
||||
## Shared source-version review (2026-10-15)
|
||||
|
||||
Reviewed `70c95bf..020d720` against the
|
||||
[source-version roadmap](source-version-checks.md), completed
|
||||
[implementation plan](implementation.md), and
|
||||
[ADR 0013](../adr/0013-shared-source-version-checks.md).
|
||||
|
||||
The root-owned mode and evidence records delegate to private per-load checking
|
||||
state. Main's local header is retained, accepted non-main headers are removed
|
||||
before placement/composition, and the destination-prefixed main rule reaches
|
||||
payload protection and typed conversion. Warnings, raw accounting, source
|
||||
contexts, and final loading reuse the existing pipeline. Four must-address
|
||||
issues remain. Findings 1 and 2 above remain completed historical findings.
|
||||
|
||||
### 3. Preserve distinct semantic shapes during evidence classification (P1)
|
||||
|
||||
Status: open.
|
||||
|
||||
Affected: `classifySecretPathsWithEvidence` and `addSecretEvidencePath` in
|
||||
[secret.go](../../internal/engine/secret.go), lines 78–104 at `020d720`.
|
||||
|
||||
Evidence candidates are stored in `map[string]Path`, keyed only by pointer text.
|
||||
Adding a later candidate overwrites an earlier semantic Path with the same
|
||||
pointer. A numeric mapping key and sequence index therefore compete for one
|
||||
classification candidate. If the last shape is incompatible with a schema map,
|
||||
`schemaSecretRoot` misses the secret field tag on the earlier mapping-shaped
|
||||
header. Final source inspection can disclose its accepted text.
|
||||
|
||||
Reproduction through `LoadLayered`:
|
||||
|
||||
- Select required mode, `ParsePath("/meta/0/version")`, and a checker accepting
|
||||
every declaration.
|
||||
- The final Go type has ordinary `Meta map[string]Header` and
|
||||
`Data.Meta map[string]SecretHeader`, with
|
||||
`SecretHeader.Version string` tagged `yaml:"version" configkit:"secret"`.
|
||||
- Main is `meta: {"0": {version: main}}` plus `data: {meta: null}`.
|
||||
- Mount two separate underlay tiers at `Keys("data")`. The first document is
|
||||
`meta: {"0": {version: KEY-HEADER-CANARY}}`; the second is
|
||||
`meta: [{version: INDEX-HEADER-CANARY}]`.
|
||||
|
||||
Both headers are accepted and consumed. Main's null replaces their remaining
|
||||
payload, so final decoding succeeds. The mapping header's source record exposes
|
||||
`Text: "KEY-HEADER-CANARY"` with `Redacted: false`, despite its schema tag.
|
||||
Reversing the underlay order redacts that same header; the classification result
|
||||
depends on which shape overwrites the candidate, not the established sensitivity
|
||||
policy. Three repeated runs reproduced this difference. The canaries are
|
||||
disposable fixture text, not real credentials.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Retain/evaluate all distinct original semantic candidate shapes before
|
||||
combining their classification results under the existing sensitivity policy.
|
||||
Preserve both evidence and payload candidates; replacing one shape with
|
||||
another must not erase applicable schema or predicate classification.
|
||||
- Reuse historical/dependency sensitivity closure and conservative failure
|
||||
disclosure. Do not fix this by exposing less evidence, forbidding numeric
|
||||
keys/sequence headers, or requiring fragment conformance to the final Go type.
|
||||
- Add a public regression covering both acquisition orders, the tagged consumed
|
||||
header, and unchanged final payload. Include a focused shape-collision test at
|
||||
the classification boundary and reuse existing neighboring-public-value tests.
|
||||
- Run source-version/sensitivity/history tests, the everyday check, and the full
|
||||
race suite. Reconcile the plan's Stage 9–12 completion evidence.
|
||||
|
||||
### 4. Preserve import opt-in before stripping source directives (P2)
|
||||
|
||||
Status: open.
|
||||
|
||||
Affected: `assembleBaseInputTrackedForRole` in
|
||||
[load_input.go](../../internal/engine/load_input.go), lines 54–87, and
|
||||
`processSourceVersion` in
|
||||
[source_version.go](../../internal/engine/source_version.go), at `020d720`.
|
||||
|
||||
Enabled source checking calls `splitImportDirectives` regardless of Imports.
|
||||
For an input with imports disabled, it then checks `disabledDirectiveDiagnostics`
|
||||
on the already stripped application payload. A valid root directive disappears
|
||||
without either traversal or the required `import.disabled` error.
|
||||
|
||||
For example, load bytes containing:
|
||||
|
||||
```yaml
|
||||
version: main
|
||||
configkit:
|
||||
imports:
|
||||
- path: missing.yml
|
||||
value: ok
|
||||
```
|
||||
|
||||
Use a version rule at `Keys("version")`, an accepting checker, and leave Imports
|
||||
false. Default mode correctly returns `import.disabled`. Both enabled modes
|
||||
incorrectly return a successful configuration, silently omitting the requested
|
||||
import. This changes import semantics even though the new mode selects only
|
||||
version checking.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Enforce the existing directive opt-in against original source content before
|
||||
stripping/consumption can hide it. Cover both ordinary main and direct underlay
|
||||
acquisition through their shared boundary. Keep genuine enabled imports and
|
||||
selected-overlay restrictions unchanged.
|
||||
- Add a focused public regression for enabled modes with Imports false and
|
||||
source-root imports. Keep default-mode behavior and safe source-local failure
|
||||
attribution; do not add a second implicit import-enabling mechanism.
|
||||
- Run source-version/directive/import tests and the everyday check.
|
||||
|
||||
### 5. Retain the complete requested path for absent declaration evidence (P2)
|
||||
|
||||
Status: open.
|
||||
|
||||
Affected: `resolveSourceVersionPath` and `sourceVersionCheckForEvidence` in
|
||||
[source_version.go](../../internal/engine/source_version.go), especially lines
|
||||
74–90 at `020d720`; evidence classification uses the same partial-path fallback.
|
||||
|
||||
Lookup retains only the prefix reached before absence or obstruction, and that
|
||||
prefix is still a valid Path. Public evidence therefore selects it instead of
|
||||
the complete configured header path. For a rule at `Keys("meta", "version")`,
|
||||
an optional underlay containing `meta: false` produces `Present: false` but
|
||||
`VersionCheck.Path == "/meta"`, not `/meta/version`. Missing an early member or
|
||||
an intermediate sequence index likewise discards the remaining suffix. The
|
||||
record identifies an existing ancestor or partial path rather than the header
|
||||
whose absence was checked.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Preserve the full requested local path when lookup stops early. Resolve the
|
||||
traversed prefix's key/index intent using actual source shape and retain
|
||||
unresolved suffix intent without guessing nonexistent container kinds.
|
||||
- Keep nearest physical Location separate from the declaration Path. Reuse the
|
||||
complete path consistently in successful/callback/failure evidence and its
|
||||
classification candidates; do not change omission policy or invent authored
|
||||
coordinates below the obstruction.
|
||||
- Add bounded regressions for a missing early member, scalar obstruction, and
|
||||
unavailable intermediate index, including successful optional and failed
|
||||
required loads. Reuse existing present-path and copy-ownership coverage.
|
||||
- Run source-version/path/source-record tests and the everyday check.
|
||||
|
||||
### 6. Apply structural-overlap rejection only to an actual declaration (P2)
|
||||
|
||||
Status: open. This is an unintended strengthening of the agreed restriction.
|
||||
|
||||
Affected: `processSourceVersion` in
|
||||
[source_version.go](../../internal/engine/source_version.go), lines 197–219 at
|
||||
`020d720`.
|
||||
|
||||
The structural-overlap test runs before handling absence and examines the
|
||||
partially resolved prefix. A valid scalar `extends` control obstructing a deeper
|
||||
header lookup is treated as a present overlapping declaration, even though
|
||||
lookup correctly classified the requested header as absent. The roadmap's
|
||||
accepted overlap restriction concerns an actual header/control overlap; missing
|
||||
headers remain governed by the selected mode.
|
||||
|
||||
Reproduction: choose if-present mode and
|
||||
`Keys("entries", "child", "extends", "version")`, with a checker accepting
|
||||
main absence. Mount main `{}` at `Keys("main")`. Mount a separate underlay at
|
||||
`Keys("under")` containing:
|
||||
|
||||
```yaml
|
||||
entries:
|
||||
base: {value: good}
|
||||
child: {extends: base}
|
||||
```
|
||||
|
||||
The underlay has no declaration, so its checker should be skipped and its valid
|
||||
inheritance should remain available. Its inheritance paths do not intersect
|
||||
main's destination-prefixed protected path. Instead, the load stops with
|
||||
`version.error` reporting a structural overlap. This rejects an otherwise valid
|
||||
optional omission, not a present value serving two structural roles.
|
||||
The same fixture succeeds in default mode.
|
||||
|
||||
Required remediation:
|
||||
|
||||
- Base declaration-overlap rejection on actual header presence. Continue
|
||||
validating malformed source controls independently, and preserve remaining
|
||||
payload/inheritance ownership checks after omission handling.
|
||||
- Keep optional non-main absence permissible and required absence a missing
|
||||
declaration failure. Do not widen the accepted restriction into a ban on all
|
||||
configured paths beneath directive spellings.
|
||||
- Add a focused load-level regression with separate mounted scopes as above,
|
||||
plus missing-required behavior. Reuse present-overlap and malformed-control
|
||||
tests to ensure genuine conflicts remain rejected.
|
||||
- Run source-version/directive/inheritance tests and the everyday check.
|
||||
|
||||
### Shared source-version review validation
|
||||
|
||||
At `020d72084d5988e958c46381724e5dad78cada3d`, using
|
||||
`go1.26.9 linux/amd64`, these existing checks passed:
|
||||
|
||||
- `GOTOOLCHAIN=local ./scripts/check.sh`.
|
||||
- `GOTOOLCHAIN=local GOPROXY=off go test -race ./...`.
|
||||
- `GOTOOLCHAIN=local GOPROXY=off go test -count=1 ./...`.
|
||||
|
||||
Four additional external-package regression probes reproduced findings 3–6 in
|
||||
an isolated temporary module outside this repository, with a local replace to
|
||||
the reviewed checkout. The full probe command was
|
||||
`GOTOOLCHAIN=local GOPROXY=off GOWORK=off go test -mod=mod -count=1 -v ./...`;
|
||||
the classification and absent-control cases also reproduced with `-count=3`.
|
||||
The module was located at `/tmp/configkit-source-version-review.L7YGaM` during
|
||||
this review; the descriptions above preserve the cases independently of that
|
||||
temporary directory.
|
||||
|
||||
No production fixes, repository regression tests, dependency updates, or release
|
||||
changes were made. Only this findings document was revised.
|
||||
|
||||
Reference in New Issue
Block a user