198 lines
8.8 KiB
Markdown
198 lines
8.8 KiB
Markdown
# 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
|
|
|
|
Status: Addressed. Plain loading rejects unsupported empty validator overrides
|
|
and invalid reference branches; supported empty overrides retain their meaning.
|
|
|
|
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.
|