Document results of a code maintainability audit for an upcoming implementation sprint
This commit is contained in:
@@ -1,311 +1,355 @@
|
||||
# Implementation Review Remediation
|
||||
# Pre-release Maintainability Remediation
|
||||
|
||||
## Status and scope
|
||||
|
||||
Status: complete. All nine findings have been addressed. Review baseline: `f42ecc70c330a399aa6dada87658694402a515ec`.
|
||||
This review covers the initial implementation against the
|
||||
[architecture policy](../policy/architecture.md), [configuration contract](../config.md),
|
||||
and [implementation plan](implementation.md). The package boundaries and principal
|
||||
lifecycle were in place at review time; the original findings below are retained
|
||||
with completion evidence. No production code was changed during the original
|
||||
review; subsequent remediation is recorded here. All findings below were reproduced through the public API or local
|
||||
validation tools, with the additional affected call sites identified by inspection.
|
||||
Status: planned; no stages have been implemented under this roadmap.
|
||||
Audit baseline: `9b147bfd96aec25e0125dcae66e136e56dc7a670`.
|
||||
|
||||
Priorities: P1 means high-impact confidentiality, isolation, or availability;
|
||||
P2 means a correctness or validation gap that also requires remediation.
|
||||
Each item below records its completed correction and validation evidence.
|
||||
Keep current contracts and implementation evidence accurate when fixing these issues;
|
||||
do not change the agreed semantics simply to accommodate the implementation.
|
||||
This roadmap owns the agreed maintainability cleanup before v0.1.0. It replaces
|
||||
the completed implementation-defect remediation record, whose findings, fixes,
|
||||
and validation evidence remain available in Git history. It does not reopen
|
||||
those defects or replace the original [implementation plan](implementation.md).
|
||||
|
||||
## 1. P1 — Honor secret tags inside supported dynamic concrete values
|
||||
The audit found worthwhile consolidation and ownership improvements, rather than
|
||||
a need to redesign ConfigKit. The offline default suite and vet passed at the
|
||||
audit baseline. That evidence is not validation of future changes or a release
|
||||
candidate.
|
||||
|
||||
Status: complete. Typed projections now retain concrete field sensitivity; final,
|
||||
baseline and pre-resolution projections feed historical/dependency classification.
|
||||
Dynamic nested, replaced, deleted and introduced-value regressions pass. A final
|
||||
audit additionally verified that validators cannot change dynamic field sensitivity
|
||||
through equal-text type replacement; the added regression failed before the fix
|
||||
and now passes. Full tests
|
||||
pass with the temporary canonical-TMPDIR workaround for issue 8; vet, build and
|
||||
whitespace checks pass.
|
||||
Read the [development guide](../development.md),
|
||||
[architecture policy](../policy/architecture.md),
|
||||
[testing policy](../policy/testing.md), and
|
||||
[documentation policy](../policy/documentation.md) before implementation.
|
||||
The [configuration contract](../config.md) owns current semantics; the
|
||||
[component overview](../internal/overview.md) owns implementation inventory.
|
||||
Release preparation remains governed by the [release policy](../policy/release.md)
|
||||
and [maintainer procedure](../release.md).
|
||||
|
||||
Evidence: [secret.go](../../secret.go), `schemaSecretRoot` stops at
|
||||
`descriptorDynamic`; [projection.go](../../projection.go), `dynamicDescriptor`
|
||||
accepts concrete structs and describes their fields, but their sensitivity is not
|
||||
carried into the classification pass.
|
||||
## Execution rules and invariants
|
||||
|
||||
Reproduction: define `Sensitive` with a string field
|
||||
`Token` tagged `yaml:"token" configkit:"secret"`. Return
|
||||
`Config{Payload: Sensitive{Token: "review-canary"}}` from defaults, where `Payload`
|
||||
is `any`, and load with `NoSource()`. Loading succeeds. `WriteEffective` prints
|
||||
`payload.token` in plaintext, and `Explain(Keys("payload", "token"))` returns that
|
||||
same text with `Redacted=false`. This does not involve a typed `Secret` reference;
|
||||
it breaks the separate field-tag classification contract for accepted Go values.
|
||||
Implement the numbered stages in order. Each stage is one reviewable unit of
|
||||
work: inspect its current owners and tests, make the change, validate it, update
|
||||
its status and evidence here, and commit the nonempty unit with a short plain
|
||||
English message. Do not create empty commits. Record any discovered behavioral
|
||||
defect separately with a reproduction rather than silently changing semantics
|
||||
under a refactoring stage.
|
||||
|
||||
Required work: preserve sensitivity from concrete dynamic descriptors through
|
||||
projection and historical classification, including values subsequently replaced
|
||||
or deleted and dependency propagation. Respect existing supported value shapes;
|
||||
do not silently ignore tags or require callers to duplicate them in a predicate.
|
||||
Keep the root public package and the existing `internal/document` and
|
||||
`internal/yaml` packages. Do not introduce dependencies, public API changes,
|
||||
provider interfaces, configurable merge policies, or a generic visitor framework.
|
||||
Private file splits do not require new packages or ADRs. Parser-specific types
|
||||
must remain behind the existing YAML adapter boundary.
|
||||
|
||||
Acceptance: canary tests for dynamic structs, nested containers, defaults and
|
||||
recorded resolution must prove redaction in effective output and final/historical
|
||||
explanation. Include replacement/deletion and derived dependencies. Typed secret
|
||||
references must remain prohibited inside dynamic values.
|
||||
Preserve the implemented contracts, particularly:
|
||||
|
||||
## 2. P1 — Reject nonregular source files before a blocking open
|
||||
- Strict composition remains distinct from intentional profile/CLI overlay.
|
||||
- Input precedence, final-only typed decoding, retained defaults, canonical
|
||||
decoding explanation events, and recorded domain resolution remain unchanged.
|
||||
- Source identity, containment, optional-import rules, limits, diagnostic codes,
|
||||
source coordinates, ordering, and early callback termination remain intact.
|
||||
- Semantic paths retain explicit key/index distinctions and historical shapes.
|
||||
- Returned values and inspection snapshots remain detached; secret values remain
|
||||
absent from history, rendering, and library-generated diagnostics.
|
||||
- Resolution runs before secret lookup and validation; ordinary loading never
|
||||
bootstraps environment variables implicitly.
|
||||
|
||||
Status: complete. Selected files, rooted imports and profiles now inspect canonical
|
||||
targets before opening and retain post-open checks. Deadline-controlled FIFO
|
||||
subprocess tests cover all three paths. Full tests pass with the issue-8 TMPDIR
|
||||
workaround; vet, build and whitespace checks pass.
|
||||
Use focused existing behavioral tests during each stage and the baseline checks
|
||||
from the development guide before committing production changes. Add tests only
|
||||
for meaningful gaps exposed by extraction, not to enforce file locations or
|
||||
private call choreography. Update current owning documentation alongside each
|
||||
move. Planned filenames below are private implementation destinations, not new
|
||||
consumer contracts.
|
||||
|
||||
Evidence: [source.go](../../source.go), `readSource`, calls `os.Open` before
|
||||
checking regular-file status. [imports.go](../../imports.go), `readImport`, and
|
||||
[profile.go](../../profile.go), `readProfileFile`, likewise open before checking.
|
||||
## Stage 1 — Remove unused overlay value snapshots
|
||||
|
||||
Reproduction: create a POSIX FIFO with no writer and call
|
||||
`Load[map[string]any](File(fifoPath))`. The child process remained blocked until
|
||||
terminated after two seconds; no nonregular-file diagnostic was returned. Byte
|
||||
limits do not help because the open itself blocks.
|
||||
Status: pending.
|
||||
|
||||
Required work: inspect canonical targets before opening and reject nonregular
|
||||
files while retaining containment and post-open checks. Apply the protection to
|
||||
selected files, imports, and profiles. This fits the existing stable-filesystem
|
||||
contract and does not require introducing a cancellation API or promising hostile
|
||||
filesystem race protection. Explicit `Reader` inputs retain their stream behavior.
|
||||
Evidence: `overlayRecord` in [overlay.go](../../overlay.go) retains `before` and
|
||||
`after` trees; profile and CLI processing construct them and
|
||||
`cloneOverlayRecords` clones them. Production consumers use the record's change
|
||||
and displaced origins, while explanation uses `historyApplication` instead.
|
||||
|
||||
Acceptance: subprocess-based FIFO regressions on supported POSIX platforms must
|
||||
finish promptly with safe nonregular-file diagnostics for all three paths. Keep
|
||||
regular-file, permitted symlink, containment, and permission tests passing.
|
||||
Work:
|
||||
|
||||
## 3. P1 — Detach mutable values introduced by resolution before publication
|
||||
1. Reconfirm all field reads before removal. Remove only the unused overlay
|
||||
snapshot fields and their construction/cloning in `overlay.go` and
|
||||
[override.go](../../override.go).
|
||||
2. Preserve `change`, `displaced`, input applications, and their ownership rules.
|
||||
Do not remove resolution snapshots or retained decoded trees: those have
|
||||
separate consumers and budget semantics.
|
||||
3. Remove locals and imports made unnecessary by the deletion. Do not replace
|
||||
the removed state with a new cache or history representation.
|
||||
|
||||
Status: complete. Added post-resolution and publication ownership copies; the latter
|
||||
preserves already validated secret state. Regressions cover nested maps/slices,
|
||||
pointers, direct/helper assignments, validator aliases, and one lookup per load.
|
||||
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.
|
||||
Acceptance: profile/CLI provenance, explanation replay, composition independence,
|
||||
and redaction tests pass unchanged in observable behavior. Source inspection
|
||||
confirms no unused overlay snapshot construction remains. An allocation benchmark
|
||||
is not required to justify removing provably unread clones.
|
||||
|
||||
Evidence: [resolution.go](../../resolution.go), `Default`/`Normalize`, directly
|
||||
assign supplied values; `runResolution` projects them but does not clone the typed
|
||||
object. [load.go](../../load.go) publishes that same object in `Result.Config`.
|
||||
## Stage 2 — Centralize parser issue translation
|
||||
|
||||
Reproduction: a reusable resolver normalizes a map field to one captured map and
|
||||
records the assignment through `Normalize`. Perform two sequential `NoSource`
|
||||
loads with this definition. Changing the first result's map changes the second
|
||||
result's map. Both loads succeed, while the second result's frozen inspection
|
||||
still shows the earlier value. There is no concurrent callback use or retained
|
||||
callback context in this reproduction.
|
||||
Status: pending.
|
||||
|
||||
Required work: establish detached typed ownership after application resolution,
|
||||
covering direct recorded assignments as well as helpers. Preserve resolved-secret
|
||||
semantics and avoid extra environment reads; cloning only defaults is insufficient.
|
||||
Ensure validators do not reintroduce equivalent-valued mutable aliases that bypass
|
||||
the projection-based mutation check and escape into published results.
|
||||
Evidence: [parse.go](../../parse.go) translates adapter issues inline, while
|
||||
`importParseDiagnostics` in [imports.go](../../imports.go) repeats the translation
|
||||
for imports and profiles.
|
||||
|
||||
Acceptance: repeated loads with shared resolver-provided maps, slices, pointers,
|
||||
and nested values remain independent of each other and captured application state.
|
||||
Keep final projection, secret lookup, and validation ordering consistent.
|
||||
Work:
|
||||
|
||||
## 4. P2 — Reconcile history replay with final-only default application
|
||||
1. Define one private `parserDiagnostics` helper in `parse.go` that maps adapter
|
||||
issues to detached public diagnostics, including related locations.
|
||||
2. Route selected-source, import, and profile parsing through it. Remove the
|
||||
import-specific translator and keep upstream parser errors private.
|
||||
3. Return canonically ordered diagnostics from the shared translation boundary.
|
||||
Preserve existing codes, messages, primary/related coordinates, and source IDs.
|
||||
Keep per-source parsing budgets and graph accounting at their current owners.
|
||||
|
||||
Status: complete. Explanation now records an explicit canonical decoding transition
|
||||
when replay differs from final typed input, preserving decoded origins and showing
|
||||
default restoration. Null/scalar-to-map profile and CLI regressions pass at parent
|
||||
and descendant paths. Full tests pass with the issue-8 TMPDIR workaround; vet,
|
||||
build and whitespace checks pass.
|
||||
Acceptance: malformed selected/imported/profile input retains safe, correctly
|
||||
attributed diagnostics. Use existing parser and import diagnostic tests; add a
|
||||
small cross-entry regression only if a source path lacks equivalent coverage.
|
||||
|
||||
Evidence: [explain.go](../../explain.go), `Explain` initializes replay from defaults
|
||||
and merges each input into it. Actual decoding applies defaults to the final
|
||||
composed/overlaid input, so intermediate replacements are not lasting default resets.
|
||||
## Stage 3 — Share bounded regular-file read mechanics
|
||||
|
||||
Reproduction: defaults contain `m: {a: 1}`, base YAML contains `m: null`, and a
|
||||
selected profile contains `m: {b: 2}`. The successful typed result correctly contains
|
||||
both `a: 1` and `b: 2`. Explanation of `/m` ends its history at only `b: 2`.
|
||||
Explanation of `/m/a` reports its removal by the base and never explains its return,
|
||||
even though its effective value is `1` with a default origin.
|
||||
Status: pending.
|
||||
|
||||
Required work: reconstruct the input-layer history without conflating it with
|
||||
incremental typed-default application. Explicitly account for effective default
|
||||
retention/reappearance and canonical decoding so history and final inspection agree.
|
||||
Preserve visibility of superseded policy-valid inputs, even when they were not
|
||||
valid for the final destination type. Do not change decoding to retain reset markers.
|
||||
Evidence: `readSource`, `importTraversal.readImport`, and `readProfileFile` repeat
|
||||
preflight stat, open, post-open stat, regular-file checks, size checks, and bounded
|
||||
reads in [source.go](../../source.go), [imports.go](../../imports.go), and
|
||||
[profile.go](../../profile.go).
|
||||
|
||||
Acceptance: cover null-to-map and scalar-to-map overlays above defaulted containers,
|
||||
including CLI overlays and descendant queries. The historical account must explain
|
||||
the effective value and origin without implying that a final-present path is absent.
|
||||
Work:
|
||||
|
||||
## 5. P2 — Replay newly created resolution paths
|
||||
1. Introduce private helpers in `source_file.go` for regular-file opening and
|
||||
bounded reading. Use narrow function parameters for stat/open operations so
|
||||
callers can supply `os.Stat`/`os.Open` or root-bound methods. Do not introduce
|
||||
an exported filesystem interface or substitute unrestricted opening for
|
||||
`os.Root` access.
|
||||
2. Keep preflight before open and post-open verification. Close every opened
|
||||
file on all paths. Preserve the existing treatment of read-only file close
|
||||
errors; root-directory close diagnostics are a separate responsibility.
|
||||
3. Return private classified failures with enough distinction for callers to
|
||||
map errors safely. Optional imports may suppress only the currently allowed
|
||||
missing-target failures at stat/open, not later read failures. Do not expose
|
||||
filesystem errors downstream through wrapping.
|
||||
4. Keep source metadata, canonicalization, containment, per-source/aggregate
|
||||
budgets, byte accounting, and caller-specific limit diagnostics outside the
|
||||
shared mechanics. Pass the computed byte limit into the reader and retain
|
||||
both the size precheck and limit-plus-one read enforcement.
|
||||
5. Reuse bounded read mechanics for reader/byte sources only where it simplifies
|
||||
existing code without changing single-use or ownership behavior. Keep secrets
|
||||
bootstrap independent: its symlink, environment, and reporting policies differ.
|
||||
|
||||
Status: complete. Replay inserts new mapping entries/subtrees and coalesces
|
||||
overlapping parent/child net operations while retaining reasons and dependencies.
|
||||
Leaf/container regressions cover nested creation, removal and overlapping records.
|
||||
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.
|
||||
Acceptance: source, import, profile, FIFO, containment, symlink, optional-missing,
|
||||
exact-budget, and reader-ownership tests pass. Retain the deadline-isolated FIFO
|
||||
regression. Cover private failure classification only where public tests cannot
|
||||
reliably induce an important filesystem failure. Run race tests for this unit.
|
||||
|
||||
Evidence: [explain.go](../../explain.go), `replaceHistoryPath`, returns immediately
|
||||
when a mapping entry is absent, even when the replacement is a newly created value.
|
||||
## Stage 4 — Establish shared document traversal ownership
|
||||
|
||||
Reproduction: load `m: {}` into a string-keyed integer map. In the resolver, set
|
||||
`m["new"] = 7` and record `/m/new` with `ChangeDerivedDefault`. Loading succeeds.
|
||||
`Explain(Keys("m", "new"))` reports effective YAML `7`, but the recorded resolution
|
||||
change has `After.Present=false` and empty YAML. Parent explanation also misses
|
||||
that insertion during replay.
|
||||
Status: pending.
|
||||
|
||||
Required work: apply recorded additions as well as replacements and deletions,
|
||||
including creation of necessary mapping ancestors when justified by the audited
|
||||
change. Handle overlapping parent/child records without inventing assignment order
|
||||
or duplicating effects; the contract records net domain changes.
|
||||
Evidence: `indexDocument`, `walkDocumentPaths`, and `projectedSensitivity` repeat
|
||||
semantic tree traversal in [load_metadata.go](../../load_metadata.go),
|
||||
[metadata.go](../../metadata.go), and [secret.go](../../secret.go). General document
|
||||
lookup currently lives in [load.go](../../load.go).
|
||||
|
||||
Acceptance: explain added map entries and nested additions at both leaf and parent
|
||||
paths, alongside deletion/replacement cases. After-values and final snapshots must
|
||||
agree while preserving reasons, dependencies, origins and redaction.
|
||||
Work:
|
||||
|
||||
## 6. P2 — Preserve semantic path kinds and reject nonexistent explanation paths
|
||||
1. Create root-private `document_walk.go` with a nil-safe preorder walker taking
|
||||
a starting `Path` and a callback receiving both path and node. Visit mappings
|
||||
in stored order and sequences in index order; construct explicit Key/Index
|
||||
segments. It visits value nodes, not synthetic mapping-key nodes.
|
||||
2. Move `indexDocument` and `documentAtPath` to this owner. Implement indexing,
|
||||
metadata shape collection, and projected sensitivity collection using the
|
||||
walker where their traversal semantics match.
|
||||
3. Document callback borrowing: callers must clone paths or nodes when retaining
|
||||
mutable state. Preserve existing defensive copies and avoid sharing mutable
|
||||
metadata across results.
|
||||
4. Keep path-bearing helpers in the root package so `internal/document` does not
|
||||
depend on the public package. Pure node representation and cloning stay in
|
||||
`internal/document`.
|
||||
5. Leave specialized traversals explicit where they need inherited origins,
|
||||
pruning, node identity deduplication, or different counting. In particular,
|
||||
input limits count mapping keys and retained-history limits count shared nodes
|
||||
once; do not implement either using the new walker without those semantics.
|
||||
|
||||
Status: complete. Metadata retains value-free effective/historical path shapes;
|
||||
queries validate explicit segment kinds, and explanation omits absent-to-absent
|
||||
operations for paths that never existed. Regressions cover wrong-kind and parsed
|
||||
numeric queries, out-of-range indices, no-output failures and removed descendants.
|
||||
Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace pass.
|
||||
Acceptance: numeric map keys versus sequence indices, escaped paths, unknown
|
||||
paths, historical shape changes, deleted descendants, dynamic secret tags, and
|
||||
metadata detachment remain protected by existing tests. No public path behavior
|
||||
or predicate coverage changes. Do not add tests that assert walker call counts.
|
||||
|
||||
Evidence: [metadata.go](../../metadata.go) indexes queries solely by `Path.String`,
|
||||
losing explicit key/index distinctions. [explain.go](../../explain.go),
|
||||
`applicationOperationPath`, considers an atomic ancestor relevant even when the
|
||||
requested descendant never existed; an absent-to-absent event then makes `Explain`
|
||||
report success.
|
||||
## Stage 5 — Separate typed projection, cloning, and comparison
|
||||
|
||||
Reproduction: load `items: [1]`, `m: {"0": 2}`, and `x: 3` into `map[string]any`.
|
||||
`Metadata.Lookup(Keys("items", "0"))` and `Metadata.Lookup(Keys("m").Index(0))`
|
||||
both succeed despite the incorrect segment kinds. Their explanation behavior
|
||||
also disagrees with metadata. `Explain(Keys("x", "missing"))` returns true and
|
||||
`WriteExplanation` succeeds, although this path never existed.
|
||||
Status: pending.
|
||||
|
||||
Required work: resolve query segment kinds against effective and historical
|
||||
container shapes before looking up textual indexes. Distinguish a genuinely
|
||||
removed path from an impossible descendant of an atomic value. Share traversal
|
||||
semantics across metadata and explanation while allowing parsed numeric tokens
|
||||
to resolve contextually.
|
||||
Evidence: [projection.go](../../projection.go) combines roughly 622 lines of
|
||||
projection, detached cloning, dynamic schema handling, cycle/limit enforcement,
|
||||
and structural comparison. Value equality deliberately ignores sensitivity,
|
||||
which validation must check separately.
|
||||
|
||||
Acceptance: explicit key/index queries reject incompatible containers; parsed
|
||||
`/items/0` and `/m/0` still resolve correctly. Never-existing scalar descendants
|
||||
return false/nil, and their writer returns an error without output. Historically
|
||||
existing descendants remain explainable after parent replacement.
|
||||
Work:
|
||||
|
||||
## 7. P2 — Reject unsupported inline pointer-to-map schemas safely
|
||||
1. Keep canonical typed projection in `projection.go`; move cloning into
|
||||
`clone.go`, shared reflection operation state and cycle helpers into
|
||||
`value_operation.go`, and value-tree comparison into `document_compare.go`.
|
||||
Preserve the descriptor-driven approach and per-operation schema cache.
|
||||
2. Rename the private equality/comparison entry points to make configuration-value
|
||||
semantics explicit. Document that location and sensitivity metadata are
|
||||
excluded, mapping order is immaterial, and sequences are compared atomically
|
||||
for change reporting. Update all callers without changing their semantics.
|
||||
3. Give validator mutation comparison a named private owner in `validation.go`.
|
||||
It must combine projected value changes and sensitivity changes, while
|
||||
retaining private resolved-secret-state auditing as an additional check.
|
||||
Do not make resolver value comparison start treating sensitivity as a value.
|
||||
4. Preserve the distinction between cloning unresolved public secret references
|
||||
and publication cloning of already validated private state. Name/document
|
||||
the clone mode explicitly rather than leaving an unexplained boolean at
|
||||
sensitive call sites.
|
||||
5. Replace custom `cmpString` calls with the standard library comparator and
|
||||
remove that helper. Do not perform unrelated test-style rewrites.
|
||||
|
||||
Status: complete. Schema construction rejects inline map pointers, including
|
||||
multiple pointer levels, and detects nonterminating inline pointer chains. Public
|
||||
regressions verify safe errors before defaults run; supported inline coverage stays
|
||||
green. Full tests pass with the issue-8 TMPDIR workaround; vet, build and whitespace
|
||||
checks pass.
|
||||
Acceptance: projection/cloning tests cover nil versus empty, custom text codecs,
|
||||
inline fields, dynamic types, cycles, and limits. Validator mutation regressions
|
||||
must still reject equal-text sensitivity changes and secret-state changes.
|
||||
Callback ownership and secret lookup tests must prove publication does not share
|
||||
borrowed values or reread the environment. Run race tests for this unit.
|
||||
|
||||
Evidence: [schema.go](../../schema.go), `describeFields`, dereferences inline field
|
||||
types before choosing the map branch, then stores a map descriptor for a field
|
||||
whose actual reflected value is a pointer. [projection.go](../../projection.go)
|
||||
subsequently applies map operations to that pointer.
|
||||
## Stage 6 — Clarify source assembly and override ownership
|
||||
|
||||
Reproduction: a configuration field declared
|
||||
`Extra *map[string]int` with `yaml:",inline"`, initialized to a non-nil map in
|
||||
defaults, panics in `Load` with `reflect: call of reflect.Value.Len on ptr to
|
||||
non-array Value`. This is an internal reflection panic, not an application-hook panic.
|
||||
Status: pending.
|
||||
|
||||
Required work: reject pointer-to-map inline declarations during schema construction
|
||||
with a safe schema diagnostic. The agreed inline shapes are structs,
|
||||
pointers-to-structs, and string-keyed maps; do not broaden them accidentally.
|
||||
Audit nearby descriptor/value-kind assumptions for the same failure class.
|
||||
Evidence: [imports.go](../../imports.go) mixes filesystem context, directives,
|
||||
graph traversal, and diagnostics; [override.go](../../override.go) combines value
|
||||
construction/validation and application. Shared composition cloning is located
|
||||
inside the override feature, while profile overlay repeats it.
|
||||
|
||||
Acceptance: nil and non-nil inline map pointers fail consistently without invoking
|
||||
default factories or panicking. Supported inline maps and inline struct pointers
|
||||
continue to load, project, clone and classify correctly.
|
||||
Work:
|
||||
|
||||
## 8. P2 — Fix the baseline test's canonical-path assumption on macOS
|
||||
1. Keep import traversal and route diagnostics in `imports.go`. Move filesystem
|
||||
context validation/canonicalization/containment to `filesystem.go`, and
|
||||
directive structures/parsing/fragment restrictions to `directives.go`.
|
||||
Give source-independent node counting an appropriate shared document owner,
|
||||
with its mapping-key accounting documented.
|
||||
2. Keep override validation and preparation in `override.go`, move parser-neutral
|
||||
value building to `override_value.go`, and application/displacement mechanics
|
||||
to `override_apply.go`.
|
||||
3. Move `cloneComposedDocument` next to `composedDocument` in `compose.go`; use it
|
||||
for both profile overlay and overrides. Ensure every retained field is copied
|
||||
according to its existing ownership contract.
|
||||
4. Keep strict composition, overlay, and history replay separate. Share small
|
||||
mechanics when identical; do not introduce merge strategy flags or one general
|
||||
merge engine. Preserve the existing profile-versus-CLI recording behavior.
|
||||
5. Simplify profile root cleanup using deferred failure-path cleanup plus an
|
||||
explicit checked success-path close. Preserve primary failure diagnostics and
|
||||
the existing successful-read root-close error diagnostic; avoid double-close
|
||||
effects. Import traversal cleanup may remain explicit where already clear.
|
||||
|
||||
Status: complete. The source test compares canonical path identity and separately
|
||||
checks the declared label. Full default and race suites pass on macOS/arm64 using
|
||||
both the default TMPDIR and an intentionally symlinked TMPDIR. Vet, build and
|
||||
whitespace checks also pass; no production canonicalization behavior changed.
|
||||
Acceptance: imports/globs, directive restrictions, source ordering, profiles,
|
||||
overrides, version restrictions, limits, and composition independence tests pass.
|
||||
No new cross-package dependencies appear. Root handles close on every path and
|
||||
profile/CLI provenance remains identical.
|
||||
|
||||
Evidence: [source_test.go](../../source_test.go),
|
||||
`TestFileSourcesReopenAndClassifyFailures`, compares `SourceInfo.Path` directly
|
||||
with the path under `t.TempDir()`. Successful sources intentionally report a
|
||||
symlink-resolved canonical path.
|
||||
## Stage 7 — Extract explicit loader lifecycle phases
|
||||
|
||||
Reproduction: both `GOPROXY=off go test ./...` and its `-race` variant fail at this
|
||||
assertion on the review host, where the temporary path traverses macOS's `/var`
|
||||
symlink. Both suites pass with `TMPDIR=/private/tmp`. Source contents are correct;
|
||||
the test expectation is wrong.
|
||||
Status: pending.
|
||||
|
||||
Required work: derive the expected canonical path through the filesystem contract,
|
||||
and separately verify the declared source label. Do not remove canonicalization
|
||||
from production code or require developers to override `TMPDIR`.
|
||||
Evidence: `loadWithSecretLookup` in [load.go](../../load.go) coordinates roughly
|
||||
200 lines of source assembly, decoding, callbacks, history, secrets, and result
|
||||
publication, including repeated creation of a composition handoff.
|
||||
|
||||
Acceptance: the ordinary documented test and race commands pass with the host's
|
||||
default temporary directory and with an intentionally symlinked temporary directory.
|
||||
Work:
|
||||
|
||||
## 9. P2 — Make the release checker's Bash prerequisite executable on macOS
|
||||
1. Keep the exported entry point and a readable linear coordinator in `load.go`.
|
||||
Move option selection to `options.go`, input assembly to `load_input.go`, and
|
||||
typed lifecycle/result publication to `load_result.go`. Move version checking
|
||||
beside the validation machinery rather than keeping it among document helpers.
|
||||
2. Use small private handoff structs for assembled input and necessary phase
|
||||
outputs, not a mutable all-purpose loader object or a long list of output
|
||||
parameters. Reuse one composition initialization helper for profile and CLI
|
||||
processing, including NoSource override handling and source byte/node totals.
|
||||
3. Preserve the exact existing order of option validation, parsing/import checks,
|
||||
schema validation, version checking, profile/CLI application, defaults factory,
|
||||
typed decoding, resolution, retained-history limit enforcement, secret lookup,
|
||||
validation, metadata/classification, and result publication.
|
||||
4. Keep diagnostic/source accumulation explicit. Warnings survive successful
|
||||
phases; errors stop dependent work and never return a usable partial Result.
|
||||
5. Preserve both ownership barriers: detach resolver-borrowed values before secret
|
||||
resolution and validation, then detach equal-valued validator replacements
|
||||
before publication while preserving validated secret state. Do not remove
|
||||
either clone as apparently redundant work.
|
||||
6. Group Result's frozen inspection state only if it clarifies this handoff; do
|
||||
not add an additional history copy or change caller ownership of Config.
|
||||
|
||||
Status: complete. Replaced mapfile with a Bash-3.2-compatible null-delimited array
|
||||
read loop. Shell orchestration tests cover spaces/newlines and fail-fast behavior.
|
||||
The complete checker passes under macOS system Bash 3.2 with cached Go 1.25.5 and
|
||||
1.26.0, including both baselines, race tests, hygiene/docs/examples and all four
|
||||
production cross-builds. New native Linux execution remains a release gate, not a
|
||||
claim made by this local check.
|
||||
Acceptance: lifecycle ordering/early-stop tests, defaults-only loading, all input
|
||||
combinations, callback ownership, secrets, warning retention, and complete
|
||||
provenance agreement pass. Keep orchestration visible enough to verify phase
|
||||
ordering by reading the coordinator. Run the complete race suite.
|
||||
|
||||
Evidence: [scripts/release-check.sh](../../scripts/release-check.sh) uses
|
||||
`mapfile -d ''`, but [the release procedure](../release.md) requires only Bash.
|
||||
The macOS system Bash is 3.2 and has no `mapfile`; the null-delimiter option also
|
||||
requires a sufficiently recent newer Bash.
|
||||
## Stage 8 — Separate explanation replay, snapshots, and rendering
|
||||
|
||||
Reproduction: `/bin/bash -c "mapfile -d '' xs </dev/null"` fails with
|
||||
`mapfile: command not found` on the supported macOS review host. The complete checker
|
||||
was not run with two configured toolchains; this isolates its shell incompatibility.
|
||||
Status: pending.
|
||||
|
||||
Required work: preferably use a Bash-3.2-compatible null-delimited read loop, keeping
|
||||
filenames safe. Alternatively require Bash 4.4+ explicitly, validate it before
|
||||
performing other work, and document the executable used to invoke the checker.
|
||||
Do not imply that the stock macOS shell satisfies undocumented requirements.
|
||||
Evidence: [explain.go](../../explain.go) combines approximately 576 lines of public
|
||||
records, replay algorithms, snapshot redaction, retained-history accounting, and
|
||||
human-readable output.
|
||||
|
||||
Acceptance: exercise the checker on supported native platforms with the documented
|
||||
shell and cached Go prerequisites. Confirm filename handling remains null-delimited
|
||||
and failures still stop the checker.
|
||||
Work:
|
||||
|
||||
## Review validation and limits
|
||||
1. Keep public explanation records and Explain coordination in `explain.go`.
|
||||
Move application/snapshot replay and history budgeting to `history.go`, safe
|
||||
snapshot projection to `snapshot.go`, and WriteExplanation with its formatting
|
||||
helpers to `explain_render.go`.
|
||||
2. Make effective rendering and explanation snapshots use the existing shared
|
||||
redaction implementation; do not create separate sensitivity rules.
|
||||
3. Retain the canonical decoding reconciliation event between input replay and
|
||||
domain resolution, overlapping resolution intent coalescing, added/deleted
|
||||
paths, effective source attribution, and historical secret classification.
|
||||
4. Document the differences between metadata origin records and value replay:
|
||||
they serve different query contracts but must agree on effective results.
|
||||
Do not replace them with a new event-store abstraction in this cleanup.
|
||||
5. Preserve writer failure behavior and ownership of retained trees. Do not cache
|
||||
explanations or optimize full-tree replay without measured need.
|
||||
|
||||
Reviewed on macOS/arm64 with Go 1.25.5. The worktree was clean at review start.
|
||||
Acceptance: explanation, effective rendering, history budget, provenance, secret
|
||||
redaction, and writer-failure tests pass without output changes. Container and
|
||||
leaf histories still reconcile with the frozen effective typed configuration.
|
||||
|
||||
- `GOPROXY=off go vet ./...` and `GOPROXY=off go build ./...` passed.
|
||||
- Default `GOPROXY=off go test ./...` and `go test -race ./...` failed at finding 8;
|
||||
no additional suite failures or race reports were observed.
|
||||
- With `TMPDIR=/private/tmp`, both complete suites passed. This is diagnostic
|
||||
evidence, not the proposed fix for the normal baseline command.
|
||||
- Temporary external Go programs reproduced findings 1–7 using public APIs.
|
||||
The FIFO reproduction used a subprocess deadline and was terminated safely.
|
||||
Reproduction values were disposable canaries, not actual credentials.
|
||||
- The system-shell probe reproduced finding 9. No release publication, full
|
||||
two-toolchain release check, new Linux execution, or extended fuzz campaign was
|
||||
performed during this review.
|
||||
## Stage 9 — Complete ownership documentation and integration validation
|
||||
|
||||
The upstream YAML adapter, node-based composition, direct typed binding, fixed
|
||||
precedence, separate explicit bootstrap, and application-owned domain lifecycle
|
||||
remain appropriate. Repair these boundary and consistency defects within that
|
||||
architecture. Re-run focused regressions, the normal baseline and race suites,
|
||||
local documentation checks, and applicable release gates before closing this roadmap.
|
||||
Status: pending.
|
||||
|
||||
## Remediation completion
|
||||
Work:
|
||||
|
||||
All nine findings are corrected and committed in individual units of work, with an
|
||||
additional issue-1 audit correction for validator sensitivity mutation. The normal
|
||||
macOS baseline no longer requires a TMPDIR workaround. The historical review and
|
||||
intermediate validation notes above retain the conditions that applied at the time.
|
||||
1. Review the final code for obsolete helpers, duplicate traversal/translation,
|
||||
unread retained state, stale comments, and imports left by earlier stages.
|
||||
Confirm file splits reflect discrete responsibilities rather than line quotas.
|
||||
2. Update the component overview to describe the actual owners and fix its stale
|
||||
claim that metadata queries are empty. Keep detailed consumer semantics in
|
||||
their existing contract; repair navigation and moved-file links throughout
|
||||
documentation, including this roadmap's evidence links if necessary.
|
||||
3. State the retained dependency direction clearly: public lifecycle coordination
|
||||
uses private source/document/schema/inspection helpers; the owned document
|
||||
representation has no dependency on the root package; only the private YAML
|
||||
adapter imports the upstream YAML package.
|
||||
4. Run the full baseline and race suites, local documentation and dependency
|
||||
boundary checks, and the complete source-candidate checker described in the
|
||||
release procedure with available qualifying toolchains. Record exact versions,
|
||||
host platform, results, and any unavailable platform evidence. Cross-builds
|
||||
are not native execution evidence.
|
||||
5. Review the accumulated diff for public API/configuration/diagnostic changes.
|
||||
Expected outcome: no consumer migration or dependency change. If any contract
|
||||
change is discovered, resolve it explicitly before closing this roadmap.
|
||||
6. Record completion evidence and commit this final nonempty unit. Release
|
||||
preparation must subsequently retire completed roadmap material and validate
|
||||
the exact committed candidate under release policy. Completing this roadmap
|
||||
does not create a tag, push refs, or authorize publication.
|
||||
|
||||
The complete source checker is validated locally with:
|
||||
Acceptance: all stages are complete with recorded validation, existing behavioral
|
||||
regressions remain protected, documentation matches the new ownership layout,
|
||||
and the worktree is clean after commits. Required release gates remain visible
|
||||
in the release procedure rather than being declared satisfied by this refactor.
|
||||
|
||||
```sh
|
||||
CONFIGKIT_BASELINE_TOOLCHAIN=go1.25.5 CONFIGKIT_NEWER_TOOLCHAIN=go1.26.0 /bin/bash scripts/release-check.sh
|
||||
```
|
||||
## Open Questions
|
||||
|
||||
This uses the available cached toolchains, runs both native baselines and the race
|
||||
suite, checks module hygiene, formatting, documentation and example boundaries,
|
||||
and cross-builds Linux/macOS on amd64/arm64. Native execution during remediation
|
||||
was macOS/arm64; new native Linux evidence and selection of current supported patch
|
||||
releases remain maintainer release gates. No release, tag, push or downstream
|
||||
application migration was performed. Retire this completed roadmap during release
|
||||
preparation according to the documentation policy.
|
||||
None required to begin implementation. The package boundaries, compatibility
|
||||
constraints, ordered units, and acceptance criteria above are decided. Routine
|
||||
private naming adjustments are acceptable when they preserve those decisions;
|
||||
new behavioral discoveries require explicit evidence and disposition.
|
||||
|
||||
Reference in New Issue
Block a user