diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 00000000..0b85f7ba --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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. diff --git a/internal/core/config/document_normalizer.go b/internal/core/config/document_normalizer.go new file mode 100644 index 00000000..4448e643 --- /dev/null +++ b/internal/core/config/document_normalizer.go @@ -0,0 +1,26 @@ +package config + +import configkit "gitea.maximumdirect.net/eric/ConfigKit" + +// documentNormalizer records leaf provenance and, while visiting a sequence, +// covers its atomic mutation boundary. ConfigKit audits sequences as whole +// values, so a record for only a descendant is insufficient. The outermost +// sequence covers nested sequences as well; unchanged visits record nothing. +type documentNormalizer struct { + *configkit.Resolution + sequence *configkit.Path +} + +func (n documentNormalizer) inSequence(path configkit.Path) documentNormalizer { + if n.sequence == nil { + n.sequence = &path + } + return n +} + +func (n documentNormalizer) Record(path configkit.Path, kind configkit.ChangeKind, reason string, dependencies ...configkit.Path) { + n.Resolution.Record(path, kind, reason, dependencies...) + if n.sequence != nil { + n.Resolution.Record(*n.sequence, kind, reason, append([]configkit.Path{path}, dependencies...)...) + } +} diff --git a/internal/core/config/document_paths.go b/internal/core/config/document_paths.go index 49e43e7f..fe974d04 100644 --- a/internal/core/config/document_paths.go +++ b/internal/core/config/document_paths.go @@ -6,7 +6,8 @@ import ( configkit "gitea.maximumdirect.net/eric/ConfigKit" ) -func normalizeDocumentPaths(document *Document, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeDocumentPaths(document *Document, context *configkit.Resolution, state *documentLoadState) { + resolution := documentNormalizer{Resolution: context} if document == nil { return } @@ -40,19 +41,17 @@ func normalizeDocumentPaths(document *Document, resolution *configkit.Resolution if profile.Output != nil { normalizeDocumentBindingReferencePaths(profile.Output, path.Key("output"), resolution, state) } - if documentStepsContainRelativeReferencePaths(profile.Steps) { - resolution.Record(path.Key("steps"), configkit.ChangeNormalization, "resolved configured paths in ordered steps") - } for laneID, lane := range profile.Artifacts { normalizeDocumentLaneReferencePaths(&lane, path.Key("artifacts").Key(laneID), resolution, state) profile.Artifacts[laneID] = lane } + stepResolution := resolution.inSequence(path.Key("steps")) for index := range profile.Steps { step := &profile.Steps[index] stepPath := path.Key("steps").Index(index) - normalizeDocumentReferencePaths(step.References, stepPath.Key("references"), resolution, state) + normalizeDocumentReferencePaths(step.References, stepPath.Key("references"), stepResolution, state) for laneID, lane := range step.Artifacts { - normalizeDocumentLaneReferencePaths(&lane, stepPath.Key("artifacts").Key(laneID), resolution, state) + normalizeDocumentLaneReferencePaths(&lane, stepPath.Key("artifacts").Key(laneID), stepResolution, state) step.Artifacts[laneID] = lane } } @@ -60,32 +59,7 @@ func normalizeDocumentPaths(document *Document, resolution *configkit.Resolution } } -func documentStepsContainRelativeReferencePaths(steps []PipelineStepDocument) bool { - for _, step := range steps { - if documentReferencesContainRelativePath(step.References) { - return true - } - for _, lane := range step.Artifacts { - if documentReferencesContainRelativePath(lane.Extract.References) || - (lane.Merge != nil && documentReferencesContainRelativePath(lane.Merge.References)) || - (lane.Normalize != nil && documentReferencesContainRelativePath(lane.Normalize.References)) { - return true - } - } - } - return false -} - -func documentReferencesContainRelativePath(references map[string]ReferenceDocument) bool { - for _, source := range references { - if source.Path != nil && *source.Path != "" && !filepath.IsAbs(*source.Path) { - return true - } - } - return false -} - -func initializeDocumentMainDirectory(resolution *configkit.Resolution, state *documentLoadState) { +func initializeDocumentMainDirectory(resolution documentNormalizer, state *documentLoadState) { if state == nil || state.mainDirectory != "" { return } @@ -102,13 +76,13 @@ func initializeDocumentMainDirectory(resolution *configkit.Resolution, state *do state.mainDirectory = cleanInvocationDirectory(".") } -func normalizeDocumentBindingReferencePaths(binding *BindingDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeDocumentBindingReferencePaths(binding *BindingDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { if binding != nil { normalizeDocumentReferencePaths(binding.References, path.Key("references"), resolution, state) } } -func normalizeDocumentLaneReferencePaths(lane *ArtifactLaneDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeDocumentLaneReferencePaths(lane *ArtifactLaneDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { if lane == nil { return } @@ -117,7 +91,7 @@ func normalizeDocumentLaneReferencePaths(lane *ArtifactLaneDocument, path config normalizeDocumentBindingReferencePaths(lane.Normalize, path.Key("normalize"), resolution, state) } -func normalizeDocumentReferencePaths(references map[string]ReferenceDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeDocumentReferencePaths(references map[string]ReferenceDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { for _, name := range sortedReferenceDocumentKeys(references) { source := references[name] sourcePath := path.Key(name).Key("path") @@ -137,7 +111,7 @@ func normalizeDocumentReferencePaths(references map[string]ReferenceDocument, pa } } -func normalizeDocumentPath(resolution *configkit.Resolution, state *documentLoadState, path configkit.Path, target **string, optional bool, reason string) { +func normalizeDocumentPath(resolution documentNormalizer, state *documentLoadState, path configkit.Path, target **string, optional bool, reason string) { if target == nil || *target == nil || **target == "" || filepath.IsAbs(**target) { return } @@ -151,11 +125,11 @@ func normalizeDocumentPath(resolution *configkit.Resolution, state *documentLoad return } if *target != nil && **target != value { - configkit.Normalize(resolution, path, target, documentValue(value), reason) + configkit.Normalize(resolution.Resolution, path, target, documentValue(value), reason) } } -func documentPathBase(resolution *configkit.Resolution, state *documentLoadState, path configkit.Path) (string, bool) { +func documentPathBase(resolution documentNormalizer, state *documentLoadState, path configkit.Path) (string, bool) { original := path if state != nil { original = state.originalPath(path) diff --git a/internal/core/config/pipeline_document.go b/internal/core/config/pipeline_document.go index 4a56f333..bbfa1d5c 100644 --- a/internal/core/config/pipeline_document.go +++ b/internal/core/config/pipeline_document.go @@ -64,14 +64,15 @@ func normalizeDocumentPipelines(document *Document, resolution *configkit.Resolu return } validateDocumentPipelinePresence(document, resolution) + normalizer := documentNormalizer{Resolution: resolution} pipelinesPath := configkit.Keys("pipelines") for _, id := range sortedPipelineDocumentKeys(document.Pipelines) { profile := document.Pipelines[id] - normalizePipelineDocument(&profile, pipelinesPath.Key(id), resolution, state) + normalizePipelineDocument(&profile, pipelinesPath.Key(id), normalizer, state) document.Pipelines[id] = profile } - document.Pipelines = normalizeDocumentMap(document.Pipelines, pipelinesPath, "pipeline identifier", resolution, state) + document.Pipelines = normalizeDocumentMap(document.Pipelines, pipelinesPath, "pipeline identifier", normalizer, state) } func validateDocumentPipelinePresence(document *Document, resolution *configkit.Resolution) { @@ -175,7 +176,7 @@ func validateDocumentPolicyPresence(policy *ValidationPolicyDocument, path confi rejectDocumentNull(resolution, path.Key("validator_failure"), policy.ValidatorFailure == nil) } -func normalizePipelineDocument(profile *PipelineDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizePipelineDocument(profile *PipelineDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { normalizePipelineDocumentString(resolution, path.Key("llm_profile"), &profile.LLMProfile, "trimmed pipeline LLM profile") normalizeDocumentBinding(&profile.Input, path.Key("input"), resolution, state) if profile.Chunk != nil { @@ -191,6 +192,7 @@ func normalizePipelineDocument(profile *PipelineDocument, path configkit.Path, r profile.Artifacts[laneID] = lane } profile.Artifacts = normalizeDocumentMap(profile.Artifacts, path.Key("artifacts"), "artifact lane identifier", resolution, state) + resolution = resolution.inSequence(path.Key("steps")) for index := range profile.Steps { step := &profile.Steps[index] stepPath := path.Key("steps").Index(index) @@ -205,7 +207,7 @@ func normalizePipelineDocument(profile *PipelineDocument, path configkit.Path, r } } -func normalizeArtifactLaneDocument(lane *ArtifactLaneDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeArtifactLaneDocument(lane *ArtifactLaneDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { normalizeDocumentBinding(&lane.Extract, path.Key("extract"), resolution, state) if lane.Merge != nil { normalizeDocumentBinding(lane.Merge, path.Key("merge"), resolution, state) @@ -213,26 +215,28 @@ func normalizeArtifactLaneDocument(lane *ArtifactLaneDocument, path configkit.Pa if lane.Normalize != nil { normalizeDocumentBinding(lane.Normalize, path.Key("normalize"), resolution, state) } + resolution = resolution.inSequence(path.Key("validators")) for index := range lane.Validators { normalizeValidatorDocument(&lane.Validators[index], path.Key("validators").Index(index), resolution) } } -func normalizeDocumentBinding(binding *BindingDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) { +func normalizeDocumentBinding(binding *BindingDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) { normalizeDocumentValue(resolution, path.Key("module"), &binding.Module, strings.TrimSpace(binding.Module), "trimmed module identifier") normalizePipelineDocumentString(resolution, path.Key("llm_profile"), &binding.LLMProfile, "trimmed binding LLM profile") binding.References = normalizeDocumentReferences(binding.References, path.Key("references"), resolution, state) + resolution = resolution.inSequence(path.Key("validators")) for index := range binding.Validators { normalizeValidatorDocument(&binding.Validators[index], path.Key("validators").Index(index), resolution) } } -func normalizeValidatorDocument(validator *ValidatorDocument, path configkit.Path, resolution *configkit.Resolution) { +func normalizeValidatorDocument(validator *ValidatorDocument, path configkit.Path, resolution documentNormalizer) { normalizeDocumentValue(resolution, path.Key("module"), &validator.Module, strings.TrimSpace(validator.Module), "trimmed validator module identifier") normalizePipelineDocumentString(resolution, path.Key("llm_profile"), &validator.LLMProfile, "trimmed validator LLM profile") } -func normalizeDocumentReferences(references map[string]ReferenceDocument, path configkit.Path, resolution *configkit.Resolution, state *documentLoadState) map[string]ReferenceDocument { +func normalizeDocumentReferences(references map[string]ReferenceDocument, path configkit.Path, resolution documentNormalizer, state *documentLoadState) map[string]ReferenceDocument { for _, name := range sortedReferenceDocumentKeys(references) { source := references[name] sourcePath := path.Key(name) @@ -246,7 +250,7 @@ func normalizeDocumentReferences(references map[string]ReferenceDocument, path c return normalizeDocumentMap(references, path, "reference name", resolution, state) } -func normalizeDocumentValue[T comparable](resolution *configkit.Resolution, path configkit.Path, target *T, value T, reason string) { +func normalizeDocumentValue[T comparable](resolution documentNormalizer, path configkit.Path, target *T, value T, reason string) { if target == nil || !resolution.IsExplicit(path) { return } @@ -256,7 +260,7 @@ func normalizeDocumentValue[T comparable](resolution *configkit.Resolution, path } } -func normalizePipelineDocumentString(resolution *configkit.Resolution, path configkit.Path, target **string, reason string) { +func normalizePipelineDocumentString(resolution documentNormalizer, path configkit.Path, target **string, reason string) { if target == nil || *target == nil || !resolution.IsExplicit(path) { return } @@ -267,7 +271,7 @@ func normalizePipelineDocumentString(resolution *configkit.Resolution, path conf } } -func normalizeDocumentMap[T any](values map[string]T, path configkit.Path, kind string, resolution *configkit.Resolution, state *documentLoadState) map[string]T { +func normalizeDocumentMap[T any](values map[string]T, path configkit.Path, kind string, resolution documentNormalizer, state *documentLoadState) map[string]T { if len(values) == 0 { return values } diff --git a/internal/core/config/sequence_normalization_test.go b/internal/core/config/sequence_normalization_test.go new file mode 100644 index 00000000..92065778 --- /dev/null +++ b/internal/core/config/sequence_normalization_test.go @@ -0,0 +1,92 @@ +package config + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestSequenceNormalizationPreservesRuntimeAndProvenance(t *testing.T) { + t.Parallel() + for _, test := range []struct { + name, body, pointer, want string + }{ + { + name: "step identifier", + body: "steps:\n - id: ' first '\n artifacts: {spells: {extract: {module: dnd/spells}}}", + pointer: "/pipelines/session/steps/0/id", + want: "first", + }, + { + name: "standalone validator", + body: "artifacts:\n spells:\n extract:\n module: dnd/spells\n validators: [{module: ' generic/valid_json '}]", + pointer: "/pipelines/session/artifacts/spells/extract/validators/0/module", + want: "generic/valid_json", + }, + { + name: "nested fields and source paths", + body: `steps: + - id: first + artifacts: + " spells ": + extract: + module: " dnd/spells " + llm_profile: " local " + validators: [{module: " generic/valid_json "}] + references: + " party ": {path: "./party.txt"} + registry: {artifact: {step: " previous ", lane: " nouns "}}`, + // Map-key moves are explained at their enclosing container, where + // the original declaration and the new canonical keys coexist. + pointer: "/pipelines/session/steps", + want: "party.txt", + }, + } { + t.Run(test.name, func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "config.yml") + if err := os.WriteFile(path, []byte("version: 5\npipelines:\n session:\n input: {module: seriatim}\n "+test.body+"\n"), 0o600); err != nil { + t.Fatal(err) + } + loaded, err := Load(path, LoadOptions{}) + if err != nil { + t.Fatal(err) + } + profile := loaded.Config.Pipelines["session"] + switch test.name { + case "step identifier": + if profile.Steps[0].ID != "first" { + t.Fatalf("step = %#v", profile.Steps[0]) + } + case "standalone validator": + if profile.Artifacts["spells"].Extract.Validators.Validators[0].Module != test.want { + t.Fatal("validator was not normalized") + } + default: + binding := profile.Steps[0].Artifacts["spells"].Extract + if binding.Module != "dnd/spells" || binding.LLMProfile != "local" || + binding.Validators.Validators[0].Module != "generic/valid_json" || + binding.References["party"].Path != filepath.Join(root, "party.txt") || + binding.References["registry"].Artifact.Step != "previous" || + binding.References["registry"].Artifact.Lane != "nouns" { + t.Fatalf("binding = %#v", binding) + } + } + var effective, explanation bytes.Buffer + if err := loaded.WriteEffective(&effective); err != nil { + t.Fatal(err) + } + if err := loaded.WriteExplanation(&explanation, test.pointer); err != nil { + t.Fatal(err) + } + if !strings.Contains(effective.String(), test.want) || + !strings.Contains(explanation.String(), test.want) || + !strings.Contains(explanation.String(), path) || + !strings.Contains(explanation.String(), "normalization") { + t.Fatalf("missing effective value or provenance:\n%s\n%s", effective.String(), explanation.String()) + } + }) + } +}