122 lines
6.3 KiB
Markdown
122 lines
6.3 KiB
Markdown
# Behavioral Equivalence and Shared Abstraction Audit
|
|
|
|
Reviewed revision: `94d1e333d422d742eb59726ad90201f4c7db9205`.
|
|
|
|
This audit examined explicit cross-path comparisons and complementary behavioral
|
|
tests, then inspected their production implementations. One focused maintenance
|
|
opportunity warrants consideration. It is not a demonstrated correctness defect
|
|
or a requirement to refactor before release.
|
|
|
|
## 1. Share typed-secret slot traversal between resolution and mutation auditing
|
|
|
|
### Evidence
|
|
|
|
The tests in [secret_reference_test.go](../../internal/engine/secret_reference_test.go)
|
|
establish complementary requirements:
|
|
|
|
- `TestSecretReferencesResolveTypedShapesOncePerName` (line 31) exercises typed
|
|
fields, optional pointers, maps of values and pointers, nil pointers, and
|
|
sequences. It verifies resolved credentials are available before validation
|
|
and each environment name is looked up once.
|
|
- `TestSecretReferenceErrorsAreSafeAndPathSpecific` (line 78) verifies secret
|
|
paths, absent/null/invalid references, optional-pointer behavior, and cached
|
|
failed lookups.
|
|
- `TestValidationCannotReplaceResolvedSecretWithSameName` (line 297) verifies
|
|
that validation cannot replace private resolved state while preserving the
|
|
projected reference name. It expects `validate.mutation` at the secret's path.
|
|
- `TestDynamicValuesDoNotEstablishSecretReferences` (line 415) verifies that
|
|
dynamic values do not acquire typed-secret semantics.
|
|
|
|
`TestSecretBindingsRemainExactAndIndependent` in
|
|
[secret_binding_test.go](../../internal/engine/secret_binding_test.go), line 123,
|
|
also verifies that the selected exact secret type belongs to the load's schema
|
|
binding. The [typed-secret contract](../config.md#typed-secret-references)
|
|
establishes both supported typed containers and the private-state validation
|
|
audit.
|
|
|
|
These tests do not compare the two walkers directly, and the private-state
|
|
mutation regression exercises a direct field rather than every container shape.
|
|
The shared responsibility is identifying schema-bound secret slots at their
|
|
semantic paths. Resolution and validation auditing intentionally perform
|
|
different operations at those slots.
|
|
|
|
### Implementation
|
|
|
|
Both walks are in
|
|
[secret_reference.go](../../internal/engine/secret_reference.go):
|
|
|
|
- `resolveSecretValue` (line 57) descends pointers, physical struct members,
|
|
string-keyed maps, slices, and arrays. It resolves names, reports failures,
|
|
assigns resolved state, and records reference names.
|
|
- `collectChangedSecretStates` (line 139), called by `changedSecretStates`,
|
|
independently implements the same container dispatch, nil handling, inline
|
|
field path handling, sorted map-key traversal, and sequence-index paths. Its
|
|
leaf operation compares private state with the earlier lookup results.
|
|
|
|
Both already share schema descriptors and `SecretBinding`, but their recursive
|
|
navigation remains duplicated. Resolution additionally copies map entries into
|
|
writable values and writes them back; auditing reads map entries directly.
|
|
[runValidation](../../internal/engine/validation.go) runs the latter after
|
|
ordinary projection/mutation checks, so the two traversals occur at different
|
|
lifecycle points.
|
|
|
|
### Opportunity and potential remediation
|
|
|
|
Consider a small engine-private walker dedicated to schema-bound secret slots.
|
|
It could own container navigation and semantic path construction, with separate
|
|
leaf operations for resolving a reference and checking its private state. This
|
|
would give changes to supported secret-container traversal one owner and reduce
|
|
the chance that resolution visits a slot that mutation auditing later misses.
|
|
|
|
Keep the existing operation-specific entry points, diagnostics, lookup cache,
|
|
optional-reference rules, and state comparisons recognizable. Make writable
|
|
versus read-only map handling explicit. Keep the two lifecycle passes separate;
|
|
sharing traversal does not mean reusing a pre-validation snapshot of slots or
|
|
combining resolution and validation into one pass.
|
|
|
|
This belongs within `internal/engine`. It needs no new public API, cross-package
|
|
reflection framework, global cache, or generic walker for cloning and projection.
|
|
|
|
### Tradeoffs
|
|
|
|
- A visitor adds indirection. Keep it specific to secrets and small enough that
|
|
the container rules and two leaf operations remain easy to read. If doing so
|
|
requires a broad configurable traversal framework, retaining the two explicit
|
|
walks is preferable.
|
|
- Map-entry addressability is an intentional difference. A common walker must
|
|
preserve resolution's copy/writeback behavior without making the audit mutate
|
|
configuration or perform unnecessary writable copies.
|
|
- Auditing must use the original per-load lookup results, without looking up
|
|
credentials again or resetting resolved references. Preserve existing nil,
|
|
inline-field, path, and exact-type semantics; do not widen dynamic-secret
|
|
support or add new rejection rules as part of consolidation.
|
|
- Sharing implementation must not make the tests depend on the walker to
|
|
discover their expected paths or values. Keep independently authored shape,
|
|
mutation, lookup-count, and non-disclosure assertions. The existing tests
|
|
establish the motivation, but do not by themselves exhaustively verify a new
|
|
shared walker across every supported shape.
|
|
|
|
## Scope and validation
|
|
|
|
The review covered source and load-entry equivalence, discovery integration,
|
|
strict composition and precedence, collection operations and history replay,
|
|
inheritance scheduling/application, schema projection/cloning, semantic paths,
|
|
metadata/explanation, secret handling, and the facade/internal boundaries.
|
|
|
|
The existing architecture already shares substantial implementation logic.
|
|
Independent reference scans in tests remain useful oracles; replacing them with
|
|
the optimized production helpers would weaken the evidence rather than improve
|
|
the architecture. Root-owned facade conversions and parser adaptation also
|
|
preserve deliberate boundaries. No additional package extraction is recommended
|
|
from this audit, and no new correctness issue was established.
|
|
|
|
Codebase graph searches, call traces, source inspection, and the existing suite
|
|
were used. No standalone duplicate-code analyzer was available on `PATH`.
|
|
With Go 1.26.10 and dependency downloads disabled, this passed:
|
|
|
|
```sh
|
|
GOTOOLCHAIN=local GOPROXY=off go test ./... -count=1
|
|
```
|
|
|
|
No production code or tests were changed, and no benchmarks were collected.
|