Record configuration normalization at sequence boundaries
This commit is contained in:
194
docs/roadmap/remediation.md
Normal file
194
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,194 @@
|
||||
# ConfigKit Adoption Review Findings
|
||||
|
||||
## Scope And Assessment
|
||||
|
||||
Reviewed the adoption commits from `c403a38` through `3010a91` against
|
||||
[the feature roadmap](configkit.md) and [the implementation plan](implementation.md).
|
||||
The main architecture is implemented: ConfigKit v0.3.0 owns composition,
|
||||
decoding, provenance, and frozen inspection; Notarius owns document semantics,
|
||||
source-aware path policy, detached runtime conversion, and catalog resolution.
|
||||
The old YAML loader and operational environment-overlay layer are removed.
|
||||
|
||||
The following findings require remediation. No application code was changed
|
||||
during this review. Reproductions used temporary configuration files and the
|
||||
built production CLI, without provider calls.
|
||||
|
||||
## Finding 1 — Record Normalization At Sequence Boundaries
|
||||
|
||||
Status: Addressed. Scoped normalization records now cover steps and validators;
|
||||
load-boundary regressions verify normalized runtime values and inspection.
|
||||
|
||||
Priority: P1. Valid configuration can fail loading.
|
||||
|
||||
Evidence: `internal/core/config/pipeline_document.go:194` records individual
|
||||
step-field changes; validator normalization at the same file's binding
|
||||
traversal similarly records individual validator fields. ConfigKit audits
|
||||
these sequence changes at the enclosing sequence boundary. The special-case
|
||||
steps record in `document_paths.go` covers relative-reference resolution,
|
||||
but not other sequence normalization.
|
||||
|
||||
This otherwise valid pipeline fails plain `config validate`:
|
||||
|
||||
```yaml
|
||||
version: 5
|
||||
pipelines:
|
||||
session:
|
||||
input: {module: seriatim}
|
||||
steps:
|
||||
- id: " first "
|
||||
artifacts:
|
||||
spells:
|
||||
extract: {module: dnd/spells}
|
||||
```
|
||||
|
||||
Observed: `resolve.unrecorded_change` at `/pipelines/session/steps`.
|
||||
Likewise, a single-step extractor with
|
||||
`validators: [{module: " generic/valid_json "}]` fails at the validators
|
||||
sequence. Accepted whitespace normalization must not depend on the presence
|
||||
of an unrelated relative reference or enclosing map-key rename.
|
||||
|
||||
Required correction:
|
||||
|
||||
- Record legitimate sequence transformations at the covering boundary required
|
||||
by ConfigKit, including nested validators and ordered steps.
|
||||
- Consolidate the relative-reference-specific workaround into a coherent
|
||||
normalization-recording approach. Do not disable the audit or record the
|
||||
entire configuration indiscriminately.
|
||||
- Preserve original-path lookup and useful normalization reasons.
|
||||
|
||||
Acceptance: the two reproductions load and normalize correctly; nested
|
||||
module/profile/selector/key changes in ordered steps also work; effective
|
||||
values match runtime values and explanation retains declaration provenance.
|
||||
Add focused load-boundary regression tests without duplicating upstream tests.
|
||||
|
||||
## Finding 2 — Preserve Explicit Presence In Document Validation
|
||||
|
||||
Priority: P2. Plain validation and runtime conversion disagree about invalid
|
||||
configuration.
|
||||
|
||||
Two concrete gaps in `internal/core/config/pipeline_document.go`:
|
||||
|
||||
- At line 405, disallowed validators are checked with `len > 0`.
|
||||
`input: {module: seriatim, validators: []}` passes plain
|
||||
`config validate`, but selected-pipeline validation rejects it because the
|
||||
runtime override is explicitly set. Output bindings have the same gap.
|
||||
- At line 428, reference source selection tests whether the path is nonblank,
|
||||
rather than whether the branch was supplied. A binding-local reference
|
||||
`{path: "", artifact: {step: previous, lane: registry}}` passes plain
|
||||
validation. Conversion in `loaded_config.go` then silently drops the
|
||||
explicitly invalid path branch and selects the artifact. Whitespace-only
|
||||
paths have the same problem.
|
||||
|
||||
Required correction:
|
||||
|
||||
- Reject an explicitly configured validator override on input/output, including
|
||||
an empty list, while retaining empty-list disabling on supported stages and
|
||||
the separately agreed empty lane-level legacy-list behavior.
|
||||
- Validate reference union branch presence separately from branch content.
|
||||
Reject a supplied empty path and reject two supplied branches; retain
|
||||
existing null rejection and valid external/generated forms.
|
||||
- Keep presence-only rules in the document boundary and share semantic
|
||||
predicates with runtime validation where practical. Do not add a second
|
||||
complete validation pass simply to compensate for these omissions.
|
||||
|
||||
Acceptance: both invalid examples fail plain loading with field-specific
|
||||
diagnostics before runtime conversion; valid empty validator overrides on
|
||||
chunk/extract/merge/normalize and valid reference forms remain accepted.
|
||||
|
||||
## Finding 3 — Restore Worker-Key Normalization Without Losing Explicitness
|
||||
|
||||
Priority: P2. The migration introduced an unplanned restriction.
|
||||
|
||||
The former `normalizeStageWorkers` trimmed keys and rejected normalized
|
||||
collisions. The new operational resolver does not normalize worker keys before
|
||||
`deriveDocumentExtractWorkers`; `validateStageWorkerKey` requires the literal
|
||||
`extract`. The following formerly supported input is now rejected:
|
||||
|
||||
```yaml
|
||||
version: 5
|
||||
concurrency:
|
||||
stage_workers:
|
||||
" extract ": 2
|
||||
```
|
||||
|
||||
Observed: `validate.error` reports the padded key as unsupported. The target
|
||||
retains whitespace-normalized worker identifiers.
|
||||
|
||||
Required correction:
|
||||
|
||||
- Normalize worker keys with deterministic collision checking and a recorded
|
||||
map transformation before applying derived defaults.
|
||||
- Preserve the original declaration's explicitness: ConfigKit presence queries
|
||||
use entry-state paths, so checking only the newly canonical key would mistake
|
||||
a moved explicit worker count for an omitted count and overwrite it.
|
||||
- Reuse the existing map/path machinery where useful rather than adding a
|
||||
parallel normalization framework.
|
||||
|
||||
Acceptance: the example retains a worker count of 2, omitted workers derive
|
||||
from effective total concurrency, explicit invalid counts still fail, and
|
||||
`extract` plus `" extract "` fails as a collision. Inspection explains the
|
||||
normalization instead of reporting a synthetic default for the explicit value.
|
||||
|
||||
## Finding 4 — Include Source Filenames In Load Diagnostics
|
||||
|
||||
Priority: P2. Import/profile failures lack actionable file attribution.
|
||||
|
||||
`internal/cli/configkit_load.go:29` calls `WriteDiagnostics` with diagnostics
|
||||
only and discards `LoadError.Sources()`. Errors display locations such as
|
||||
`(source-1:6:7)` without any filename or source-ID legend. With composed
|
||||
configurations, an operator cannot identify the file to edit from that output.
|
||||
Stage 8 explicitly called for safe source context using source records.
|
||||
|
||||
Required correction:
|
||||
|
||||
- Continue using ConfigKit's canonical diagnostic renderer and add a small,
|
||||
deterministic source-ID-to-file/label presentation using its public source
|
||||
records. A source legend is sufficient; do not rebuild a diagnostic framework.
|
||||
- Quote/escape source labels safely, and keep all diagnostics on stderr.
|
||||
Do not print configuration contents or wrap the same failure repeatedly.
|
||||
- Cover an error originating in an imported file or selected profile, not only
|
||||
a main-file failure.
|
||||
|
||||
Acceptance: stderr identifies the actual declaring filename alongside the
|
||||
diagnostic's source ID, line, and column; stdout remains empty on failure;
|
||||
no credentials, raw YAML, or unescaped terminal controls are added.
|
||||
|
||||
## Finding 5 — Finish Current Documentation And Comment Migration
|
||||
|
||||
Priority: P2 for the unusable operator example; P3 for the stale comment.
|
||||
|
||||
`docs/config.md:539` explains how to enable the combat semantic validator,
|
||||
but five entries in its YAML snippet still use scalar validator shorthand.
|
||||
Version 5 requires objects. Copying even its first scalar entry produces
|
||||
`decode.type`: “configuration value must be a mapping.”
|
||||
|
||||
`internal/core/config/load.go:18` also still describes the production loader
|
||||
as under construction and claims commands use the deleted version 4 loader.
|
||||
|
||||
Required correction:
|
||||
|
||||
- Convert every validator in that current example to the canonical
|
||||
`module:` object form, preserving order and retry settings.
|
||||
- Check other current snippets for the same migration omission, excluding
|
||||
intentional historical/migration examples.
|
||||
- Replace the obsolete loader comment with its implemented responsibility.
|
||||
|
||||
Acceptance: a minimal v5 configuration containing the documented validator
|
||||
chain passes production selected-pipeline validation offline, and current
|
||||
comments no longer describe a dual-loader transition.
|
||||
|
||||
## Verification
|
||||
|
||||
Already passed during review:
|
||||
|
||||
- `go test ./...`
|
||||
- `go vet ./...`
|
||||
- Production CLI build.
|
||||
- Selected-pipeline validation of both maintained D&D examples, plus the complete
|
||||
example with the development configuration profile.
|
||||
- Effective/explanation inspection smoke checks.
|
||||
|
||||
The existing green suite does not cover the failing edge cases above. After
|
||||
remediation, add the focused behavioral coverage described per finding, rerun
|
||||
the full checks, and validate the corrected documentation example. No live
|
||||
LLM tests are needed.
|
||||
Reference in New Issue
Block a user